Formulary Documentation: Adding Missing Formulas to Docstrings #1041

Tiger-Du · 2021-02-25T01:02:15Z

I have added a changelog entry for this pull request.

If adding new functionality, I have added tests and
docstrings.

I have fixed any newly failing tests.

This PR is for Issue #935.

I went through the documentation of plasmapy.formulary, and from what I can tell, outside of plasmapy.formulary.braginskii.py, the only two (user-facing) functions that lack formulas are dimensionless.quantum_theta and dimensionless.beta. These quantities are simple ratios, but I believe they still should get complete documentation. Formulas for these two functions are added in this PR.

I plan to add formulas to the docstrings in braginskii.py in this PR although these may be more complicated to document because of multiple supported models of classical transport.

On a related note, I am not sure whether the methods of classes, such as the Classical Transport class in braginskii.py, should be documented in the same way (or at all) as standalone / free functions, and I would like to hear what other contributors think.

Sync

codecov · 2021-02-25T01:04:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.86%. Comparing base (6d5651c) to head (6872106).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1041      +/-   ##
==========================================
+ Coverage   96.21%   96.86%   +0.65%     
==========================================
  Files          60       70      +10     
  Lines        5417     6864    +1447     
==========================================
+ Hits         5212     6649    +1437     
- Misses        205      215      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

StanczakDominik · 2021-02-25T09:35:53Z

On a related note, I am not sure whether the methods of classes, such as the Classical Transport class in braginskii.py, should be documented in the same way (or at all) as standalone / free functions, and I would like to hear what other contributors think.

If I remember correctly, the functions call the ClassicalTransport methods and not the other way around; I would suggest bluntly referencing the ones with the actual implementation (e.g. "Wraps classicaltransport.this_method") from the wrappers :)

StanczakDominik

Good stuff! One note, though :)

plasmapy/formulary/dimensionless.py

Tiger-Du · 2021-02-28T05:32:25Z

@StanczakDominik - Hi, Dominik. Thank you for reviewing this PR. I made the changes you suggested in dimensionless.py by adding "See Also" sections and the formula for thermal energy in the "Notes" section. I also added formulas (or introduced them) in the docstrings of braginskii.py. I am deciding to add formulas to both the methods and the free functions here because even though they are (very) similar, they are separate functionality. I do not think we should spend too much effort editing braginskii.py in this PR because it is due for refactoring. But I think the changes made so far to braginskii.py make it more presentable for 0.6.0. Hopefully, this PR could be quick to merge! :)

StanczakDominik · 2021-02-28T05:39:36Z

I am deciding to add formulas to both the methods and the free functions here because even though they are (very) similar, they are separate functionality.

One worry I have about that is that if this works as I remembered and one calls the other, changing - for whatever possible reason, suppose we made a mistake and were off by sqrt2 for concreteness - one implementation will propagate changes in results, but not the documentation, into the other. I know I could and likely would easily forget that, which is why I advocated for the see also or thin wrapper for (as we do with aliases) approach earlier.

But it's a worry I can live with, I think. I can get you a review sometime after breakfast :)

Tiger-Du · 2021-02-28T06:06:50Z

@StanczakDominik - Good point. I am not very familiar with wrappers, so maybe you could suggest some code for that. But perhaps this problem will end up being moot in the future if some significant refactoring of braginskii.py happens. We might also address this problem with something like #946 but for docstring formulas.

Also, forgot to mention, but I plan to add formulas to the docstrings in formulary.magnetostatics.py in this PR too.

StanczakDominik · 2021-03-01T18:34:12Z

Gah, I'm sorry, that turned out to be a off-by-one-breakfast error combined with a wrong meal bug... getting to it now!

When I say "wrapper", I mean something like how this:

PlasmaPy/plasmapy/formulary/braginskii.py

Lines 740 to 794 in 6a161f0

    
           @validate_quantities 
        
           def resistivity( 
        
               T_e, 
        
               n_e, 
        
               T_i, 
        
               n_i, 
        
               ion, 
        
               m_i=None, 
        
               Z=None, 
        
               B: u.T = 0.0 * u.T, 
        
               model="Braginskii", 
        
               field_orientation="parallel", 
        
               mu=None, 
        
               theta=None, 
        
               coulomb_log_method="classical", 
        
           ) -> u.Ohm * u.m: 
        
               """ 
        
               Calculate the resistivity. 
        
               Notes 
        
               ----- 
        
               The resistivity here is defined similarly to solid conductors, and thus 
        
               represents the classical plasmas' property to resist the flow of 
        
               electrical current. The result is in units of ohm * m, so if you 
        
               assume where the current is flowing in the plasma (length and 
        
               cross-sectional area), you could calculate a DC resistance of the 
        
               plasma in ohms as resistivity * length / cross-sectional area. 
        
               Experimentalists with plasma discharges may observe different V = IR 
        
               Ohm's law behavior than suggested by the resistance calculated here, 
        
               for reasons such as the occurrence of plasma sheath layers at the 
        
               electrodes or the plasma not satisfying the classical assumptions. 
        
               Returns 
        
               ------- 
        
               astropy.units.quantity.Quantity 
        
               """ 
        
               ct = ClassicalTransport( 
        
                   T_e, 
        
                   n_e, 
        
                   T_i, 
        
                   n_i, 
        
                   ion, 
        
                   m_i, 
        
                   Z=Z, 
        
                   B=B, 
        
                   model=model, 
        
                   field_orientation=field_orientation, 
        
                   mu=mu, 
        
                   theta=theta, 
        
                   coulomb_log_method=coulomb_log_method, 
        
               ) 
        
               return ct.resistivity

is a very thin wrapper for this:

PlasmaPy/plasmapy/formulary/braginskii.py

Lines 457 to 491 in 6a161f0

    
               @property 
        
               @validate_quantities 
        
               def resistivity(self) -> u.Ohm * u.m: 
        
                   """ 
        
                   Calculate the resistivity. 
        
                   Notes 
        
                   ----- 
        
                   The resistivity here is defined similarly to solid conductors, and thus 
        
                   represents the classical plasmas' property to resist the flow of 
        
                   electrical current. The result is in units of ohm * m, so if you 
        
                   assume where the current is flowing in the plasma (length and 
        
                   cross-sectional area), you could calculate a DC resistance of the 
        
                   plasma in ohms as resistivity * length / cross-sectional area. 
        
                   Experimentalists with plasma discharges may observe different V = IR 
        
                   Ohm's law behavior than suggested by the resistance calculated here, 
        
                   for reasons such as the occurrence of plasma sheath layers at the 
        
                   electrodes or the plasma not satisfying the classical assumptions. 
        
                   Returns 
        
                   ------- 
        
                   astropy.units.quantity.Quantity 
        
                   """ 
        
                   alpha_hat = _nondim_resistivity( 
        
                       self.hall_e, self.Z, self.e_particle, self.model, self.field_orientation 
        
                   ) 
        
                   tau_e = 1 / fundamental_electron_collision_freq( 
        
                       self.T_e, self.n_e, self.ion, self.coulomb_log_ei, self.V_ei 
        
                   ) 
        
                   alpha = alpha_hat / (self.n_e * e ** 2 * tau_e / m_e) 
        
                   return alpha

Though yeah, I think any refactor we'd make here would start by deprecating the heck out of these functions, as - looking at this for the first time in a while - that's just terrible design 😅

StanczakDominik

The docstrings look good, I'd be happy to merge it now, but I figured I'd wait and see if you want to discuss the comments a little :) If not (future PRs, and stuff), just say so and I can hit merge any time.

plasmapy/formulary/braginskii.py

StanczakDominik · 2021-03-01T18:54:50Z

plasmapy/formulary/braginskii.py

        Calculate the thermal conductivity for ions.

+        The ion thermal conductivity (:math:`\kappa`) of a plasma is defined by


Just a thought - one key piece of intuition I was missing while reviewing these three years ago was the understanding that the way these are defined is a transport law like $\vec{q} = -\kappa * \nabla T$, q being the heat flux caused by the temperature gradient. What transport theory actually does is provide estimates, based on reasonable assumptions, for the magnitude of $\kappa$ and other such coefficients. And then you can have different variants, including curvature effects (thus, neoclassical), or turbulence (thus, anomalous).

So yeah, despite wondering a little about the word "defined", I think this is a good change, but I wonder whether we shouldn't rewrite the docs for the whole transport module a little, to account for that...

gah, of course we should do that, we should take a hammer to the whole thing, after all. I'll make a note to do that when we refactor it.

But at the same time, those transport laws are very general and don't necessarily apply only to classical transport. So it might be a moot point.

Interesting points. We could change "defined" if users or contributors might be confused by this term. Perhaps something like "... is related by the formula ..." or "This function computes ... by the formula ...". But I think that we should discuss this and figure this out in an Issue or Discussion so that the documentation can be standardized.

Good point, let's come back here from #1044 later :)

…, no need to run the precommit in this other directory I cloned...

StanczakDominik · 2021-03-08T12:36:28Z

That should do it, merging! Thanks, @Tiger-Du :)

Tiger-Du added 3 commits October 19, 2020 13:00

Merge pull request #1 from PlasmaPy/master

299b9db

Sync

Update dimensionless.py

70b0d7a

Update dimensionless.py

3240297

Tiger-Du added 5 commits February 24, 2021 19:07

Update dimensionless.py

b2e430c

Update dimensionless.py

a691bea

Update dimensionless.py

ecdba1e

Update dimensionless.py

b901f9a

Create 1041.doc.rst

b3686bb

StanczakDominik requested changes Feb 25, 2021

View reviewed changes

plasmapy/formulary/dimensionless.py Show resolved Hide resolved

Tiger-Du mentioned this pull request Feb 25, 2021

Braginskii.py and the ClassicalTransport Class #1044

Open

Tiger-Du added 8 commits February 27, 2021 17:33

Update dimensionless.py

0ab6ec2

Update dimensionless.py

5fcfa6e

Update dimensionless.py

26b82e5

Update dimensionless.py

85ba880

Update dimensionless.py

14f9b7e

Update braginskii.py

52b41e3

Update braginskii.py

8acea23

Update braginskii.py

e782e30

Tiger-Du requested a review from StanczakDominik February 28, 2021 05:15

StanczakDominik approved these changes Mar 1, 2021

View reviewed changes

Tiger-Du and others added 4 commits March 4, 2021 14:38

Update braginskii.py

7a3c490

Update braginskii.py

b9328ab

Update braginskii.py

0e9c779

Add todo extension to Sphinx

a193786

This is what I get for thinking this gonna be a quick and easy change…

6872106

…, no need to run the precommit in this other directory I cloned...

StanczakDominik enabled auto-merge (squash) March 8, 2021 12:36

StanczakDominik merged commit c73b4ef into PlasmaPy:master Mar 8, 2021

StanczakDominik added this to the 0.6.0 milestone Mar 12, 2021

rocco8773 added a commit to rocco8773/PlasmaPy that referenced this pull request Mar 14, 2021

review change log entry(ies) for PR PlasmaPy#1041

d50ff97

Tiger-Du mentioned this pull request Jan 1, 2022

Add docstrings to functions in braginskii.py #410

Open

6 tasks

namurphy added docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.formulary Related to the plasmapy.formulary subpackage labels May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formulary Documentation: Adding Missing Formulas to Docstrings #1041

Formulary Documentation: Adding Missing Formulas to Docstrings #1041

Tiger-Du commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

StanczakDominik commented Feb 25, 2021

StanczakDominik left a comment

Tiger-Du commented Feb 28, 2021

StanczakDominik commented Feb 28, 2021

Tiger-Du commented Feb 28, 2021

StanczakDominik commented Mar 1, 2021

StanczakDominik left a comment

StanczakDominik Mar 1, 2021

StanczakDominik Mar 1, 2021

Tiger-Du Mar 4, 2021

StanczakDominik Mar 8, 2021

StanczakDominik commented Mar 8, 2021

		Calculate the thermal conductivity for ions.

		The ion thermal conductivity (:math:`\kappa`) of a plasma is defined by

Formulary Documentation: Adding Missing Formulas to Docstrings #1041

Formulary Documentation: Adding Missing Formulas to Docstrings #1041

Conversation

Tiger-Du commented Feb 25, 2021 • edited Loading

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

StanczakDominik commented Feb 25, 2021

StanczakDominik left a comment

Choose a reason for hiding this comment

Tiger-Du commented Feb 28, 2021

StanczakDominik commented Feb 28, 2021

Tiger-Du commented Feb 28, 2021

StanczakDominik commented Mar 1, 2021

StanczakDominik left a comment

Choose a reason for hiding this comment

StanczakDominik Mar 1, 2021

Choose a reason for hiding this comment

StanczakDominik Mar 1, 2021

Choose a reason for hiding this comment

Tiger-Du Mar 4, 2021

Choose a reason for hiding this comment

StanczakDominik Mar 8, 2021

Choose a reason for hiding this comment

StanczakDominik commented Mar 8, 2021

Tiger-Du commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading