[PROTO] Framework for lite "fast" formulary functionality #1098

rocco8773 · 2021-04-02T03:57:07Z

This is a prototype for developing a framework to implement lite ("lightweight") formulary functionality whose design intent is for computation. The current formulary functionality is constructed around Astropy's units for user convenience and to reduce calculation errors from improper unit use. This is great, but yields very slow executing code. The goal here is to develop a framework that still has our traditional formulary functionality but also an intuitive lite version for computation.

This prototype is developed for the thermal_speed function. The approach this prototype takes is...

A lightweight thermal speed function is developed as thermal_speed_lite. The formula this function has is based on SI units, but there is no argument validations (values or units). The assumption here is the user knows what is going and can insure proper values are passed to the function. The function is also decorated with numba.njit.
The previous thermal_speed function exists as it always has, but now utilizes thermal_speed_lite within the code for calculation.
A decorator add_lite is developed to bind thermal_speed_lite to thermal_speed as thermal_speed.lite, as well as update thermal_speed's docstrings to indicate the addition of lite.

This all works something like...

@preserve_signature  # this is needed since njit destroys the signature
@numba.njit
def thermal_speed_lite(...):
    ...

@add_lite(thermal_speed_lite)
@validate_quantities(...)
@particle_inpute
def thermal_speed(...):
    ...

The use now looks like...

>>> T = 5 * u.K
>>> par = Particle("He+)
>>> thermal_speed(T, par)
<Quantity 144.13705752 m / s>

>>> %timeit thermal_speed(T, par)
717 µs ± 34.7 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

>>> # now using lite
>>> coef = thermal_speed.coefficients[3]["most_probable"]  # coefficients is also bound using add_lite
>>> thermal_speed.lite(T.value, par.mass.value, coef)
144.13705751725402

>>> %timeit thermal_speed.lite(T.value, par.mass.value, coef)
71.7 µs ± 3.16 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

>>> # better performance can be achieved if units don't have to be used at all
>>> T_unitless, mass_unitless = T.value, par.mass.value
>>> %timeit thermal_speed.lite(T_unitless, mass_unitless, coef)
397 ns ± 5.09 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

See further discussions in #1090

…ly document imported attributes/variables

… attributes/variables are documented

…o __all__

…lite using add_lite; incorporate thermal_speed_lite into the thermal_speed calculation

…e decoration

StanczakDominik · 2021-04-02T03:59:11Z

posted a minute ago

Good timing on my part! 😉

codecov · 2021-04-02T04:12:18Z

Codecov Report

Merging #1098 (013b613) into master (5dd2547) will decrease coverage by 0.03%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master    #1098      +/-   ##
==========================================
- Coverage   96.91%   96.88%   -0.04%     
==========================================
  Files          70       70              
  Lines        6928     6956      +28     
==========================================
+ Hits         6714     6739      +25     
- Misses        214      217       +3

Impacted Files	Coverage Δ
plasmapy/utils/decorators/helpers.py	`96.72% <90.00%> (-3.28%)`	⬇️
plasmapy/formulary/parameters.py	`98.68% <92.85%> (-0.41%)`	⬇️
plasmapy/utils/decorators/__init__.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dd2547...013b613. Read the comment docs.

StanczakDominik

The goal here is to develop a framework that still has our traditional formulary functionality but also an intuitive lite version for computation.

I personally hate the fact that we're still enjoying zero-ver, but already have traditions around functionality 😉

Looks great! A bunch of minor text fix suggestions, but I have no complaints about the implementation!

StanczakDominik · 2021-04-02T09:14:33Z

plasmapy/formulary/parameters.py

+"""
+Dictionary of various coefficients used in calculating the
+`~plasmapy.formulary.parameters.thermal_speed`.
+"""


Minor rewrite:

Suggested change

"""

Dictionary of various coefficients used in calculating the

`~plasmapy.formulary.parameters.thermal_speed`.

"""

"""

Dictionary of various coefficients used for

`~plasmapy.formulary.parameters.thermal_speed`, with the dimensionality

as the key.

"""

StanczakDominik · 2021-04-02T09:15:53Z

plasmapy/formulary/parameters.py

+ A lite weight version of `thermal_speed` intended for computational used
+ and, thus, does not do any argument validation/conditioning.


Minor rewrite:

Suggested change

A lite weight version of `thermal_speed` intended for computational used

and, thus, does not do any argument validation/conditioning.

A lightweight version of `thermal_speed` intended for computational use

which does not do any argument validation/conditioning.

StanczakDominik · 2021-04-02T09:16:56Z

plasmapy/formulary/parameters.py

- **Aliases:** `vth_`
+ **Aliases:** `~plasmapy.formulary.parameters.vth_`
+
+ **Lightwieght Version:** `~plasmapy.formulary.parameters.thermal_speed_lite`


Typo fix

Suggested change

**Lightwieght Version:** `~plasmapy.formulary.parameters.thermal_speed_lite`

**Lightweight Version:** `~plasmapy.formulary.parameters.thermal_speed_lite`

and wow, with the suggestions interface and the stars around it, this really jumps out at you like a coke ad.

StanczakDominik · 2021-04-02T09:20:54Z

plasmapy/formulary/parameters.py


 # different methods, as per https://en.wikipedia.org/wiki/Thermal_velocity
 try:
- coef = _coefficients[ndim]
+ coeff = thermal_speed_coefficients[ndim]


Just taking the opportunity to avoid overwriting the var name later with a minor refactor

Suggested change

coeff = thermal_speed_coefficients[ndim]

coeffs_for_dim = thermal_speed_coefficients[ndim]

StanczakDominik · 2021-04-02T09:21:11Z

plasmapy/formulary/parameters.py

 except KeyError:
 raise ValueError("{ndim} is not a supported value for ndim in thermal_speed")
 try:
- coef = coef[method]
+ coeff = coeff[method]


continues minor refactor

Suggested change

coeff = coeff[method]

coeff = coeffs_for_dim[method]

StanczakDominik · 2021-04-02T09:24:22Z

plasmapy/formulary/parameters.py



 vth_ = thermal_speed
-""" Alias to :func:`thermal_speed`. """
+""" Alias to :func:`~plasmapy.formulary.parameters.thermal_speed`. """


Works nicely with #1099, too! 😄

Yeah, I ran across this same thing when working on this PR. I actually thought I already made this change. I think we need to get in the practice of using the explicit link since `thermal_speed` would just look for the variable in the current global() scope.

StanczakDominik · 2021-04-02T09:25:38Z

plasmapy/utils/decorators/helpers.py

@@ -142,3 +142,51 @@ def wrapper(*args, **kwargs):
 wrapper.__signature__ = inspect.signature(f)

 return wrapper
+
+
+def add_lite(flite, attrs=None, scope=None):


Maybe "register_lite", come to think of it? I think "register" has a pretty common meaning for decorators.

When I think "register" I'm think it's registering the functionality with some kind of table/database. How about bind_lite? That's closer to what it is actually doing.

That sounds great!

StanczakDominik · 2021-04-02T09:28:05Z

plasmapy/utils/decorators/helpers.py

+ def wrapper(*args, **kwargs):
+ return f(*args, **kwargs)
+
+ wrapper_doc = inspect.cleandoc(getattr(wrapper, "__doc__", """"""))


Suggested change

wrapper_doc = inspect.cleandoc(getattr(wrapper, "__doc__", """"""))

wrapper_doc = inspect.cleandoc(getattr(wrapper, "__doc__", ""))

since

> """""" == "" True

plasmapy/utils/decorators/helpers.py

StanczakDominik · 2021-04-02T09:35:02Z

requirements/requirements.txt

@@ -5,6 +5,7 @@ h5py >= 2.8
 lmfit >= 1.0.1
 matplotlib >= 2.0
 mpmath >= 1.0.0
+numba


Once you start adding tests for this, tox-minimal-37 will complain since it has its own list of dependencies. I'm adding that in tox.ini for you.

I thought we already had numba for some reason, so I was surprised when all the tests started failing because of this. Do you have any restrictions related to which numba version we should take?

Yeah, we experimented with adding it in #953; at the time, Py3.9 was released, numba completely broke on it with its bytecode changes, and we didn't want to add version-dependent numba-or-nonumba logic. 0.53 is out now; things should be fine, as py39 will just (I think...) default to that as the only compatible dependency. Though we should look at the CI logs and check which runners get which version, just in case.

I don't have a clean lower bound on which numba versions are fine, though.

pheuer

This looks great @rocco8773 !! I'm so excited about this. I've suggested a few changes - let me know what you think.

The documentation looks great: the only part I don't love is that the lite versions of the functions appear alongside the main functions and the aliases in the list of functions. But, I think we should just accept that for now. When we figure out a solution for the aliases, hopefully that will improve this situation too.

pheuer · 2021-04-02T13:17:57Z

plasmapy/formulary/parameters.py

+ thermal_speed_lite,
+ attrs=[("coefficients", "thermal_speed_coefficients")],
+ scope=globals(),
+)
 @check_relativistic
 @validate_quantities(
 T={"can_be_negative": False, "equivalencies": u.temperature_energy()},
 mass={"can_be_negative": False, "can_be_nan": True},
 )
 @particles.particle_input
 def thermal_speed(


The docstring for thermal_speed includes a table of the coefficients: maybe we should add a link next to that to the thermal_speed_coefficients variable to make more clear that these contain the same information.

Agreed! I'll get to this. I haven't refined and the docstrings because I didn't want to spend the time on it until the framework was approved.

pheuer · 2021-04-02T13:24:07Z

plasmapy/utils/decorators/helpers.py

@@ -142,3 +142,51 @@ def wrapper(*args, **kwargs):
 wrapper.__signature__ = inspect.signature(f)

 return wrapper
+
+
+def add_lite(flite, attrs=None, scope=None):


I think this might be better split into two functions, eg. register_lite and register_attrs. Or, maybe flite should also be a kwarg? I'm just thinking that currently attaching an attribute without also attaching a lite function would not be supported, but that seems like it should be possible.

Hmm, what use case would you see for that?

I don't see one now: just following the "one function one job" principle.

It is doing "one job" I added the ability to add other attributes than "lite", because sometimes you need additional attributes to properly use "lite". For example, for thermal_speed.lite to properly work you also need the coefficients. So, think of add_lite as binding all required "lite" functionality (not just the lite function) to get <func>.lite to work.

Additionally, to get the documentation to come out correctly and cleanly then the lite function and required attributes needs to be added at the same time.

I see that. I guess I'm fine with this implementation now: if we need to add attributes separately in the future I guess we can always break this function apart then.

pheuer · 2021-04-02T13:25:09Z

plasmapy/utils/decorators/helpers.py

+ """
+ if attrs is None:
+ attrs = []
+ elif scope is None:


Is there a reason to not just set scope=globals() by default?

I suspect this part could be optimized out given some tinkering. Maybe sys.modules(__module__) combined with importlib and inspect.getmembers in some funny way...

But at this point, if you just bluntly set globals as a default from helpers.py, it's going to "inherit the scope of helpers.py" rather than the local environment of the function that's being registered.

This party is trickier than it seems, I think!

Got it - that makes sense (as much as anything regarding scopes does)

But at this point, if you just bluntly set globals as a default from helpers.py, it's going to "inherit the scope of helpers.py" rather than the local environment of the function that's being registered.

Yes, this is what was happening when globals() was the default value.

pheuer · 2021-04-02T13:28:09Z

plasmapy/formulary/parameters.py

+@preserve_signature
+@njit
+def thermal_speed_lite(T, mass, coeff):
+ """


This docstring should still provide information on the parameters and returns so those appear in the documentation. Right now in the docs there's no indication that this function doesn't take units, or where one should look to determine coeff

pheuer · 2021-04-02T15:45:03Z

@StanczakDominik Let's continue the conversation from #1099 about separately documenting lite functions here. It sounds like we have three possible solutions in mind so far:

Leave lite functions alone: let them appear right next to the parent functions in the docs.
- Pros: Less work, no additional code
- Cons: Potentially doubles the number of visible functions. Could be confusing when some functions have lite versions but others don't yet.
Use "semi-dynamically generated module" wizardry to sort "_lite" functions into a separate module under the hood (like Semi-dynamically generated aliases submodule #1099 does for aliases).
- Pros: Separates the lite functions visually in the docs.
- Cons: Possible unintended consequences? No one understands sphinx.
Remove lite functions from all, document them manually in the docstrings of their main functions (possibly with a decorator that automatically inserts the lite function docstring at the end of the main function).
- Pros: Cleaner documentation (for the main functions)
- Cons: Harder to find documentation on lite functions? Sphinx might complain about non-private functions being left out of __all__?

Obligatory tag @namurphy because trade-off analysis 😃

StanczakDominik · 2021-04-02T15:47:33Z

See, Nick? We're learning! 😸

The con for 1 is pretty heavy. 2 - hard to tell; Sphinx is a riddle. 3 would take a quick prototype that I would absolutely do right now if I were less conscious of my current procrastination driven development binge.

I would say 3 is acceptable if the lite functions appear in the index. Since they are actually separate objects, I suspect they might. But again, (:egypt: :bird: :lion: :woman:) is a riddle.

rocco8773 · 2021-04-02T19:27:32Z

The goal here is to develop a framework that still has our traditional formulary functionality but also an intuitive lite version for computation.

I personally hate the fact that we're still enjoying zero-ver, but already have traditions around functionality 😉

I think this is why I've become more in favor of the date base versioning. I think plasmapy will always have a combination of mature and development functionality. That's why I consider it perpetual beta.

Looks great! A bunch of minor text fix suggestions, but I have no complaints about the implementation!

Great! I wanted to get everyone's opinion on the framework before spending the time to put the bells and whistles on all the functionality and docstrings. I can start refining everything now.

rocco8773 · 2021-04-02T19:42:46Z

Leave lite functions alone: let them appear right next to the parent functions in the docs.

Pros: Less work, no additional code

Cons: Potentially doubles the number of visible functions. Could be confusing when some functions have lite versions but others don't yet.

I don't see that as a con. <func>_lite is part of the public api and thus should be documented. Hiding it from the documentation significantly reduces discoverability, which is a major pain for users. Since <func>_lite is apart of the public api, a use would see it, go look for it in the documentation, and then not be able to find it. This is BAD! I'd rather have everything listed than not have something discoverable.

Use "semi-dynamically generated module" wizardry to sort "_lite" functions into a separate module under the hood (like Semi-dynamically generated aliases submodule #1099 does for aliases).

Pros: Separates the lite functions visually in the docs.

Cons: Possible unintended consequences? No one understands sphinx.

This could be done "simply" for aliases, but I don't like it for <func>_lite stuff. We'd need to develop something along the lines of automodsumm to accomplish this, see my #1099 (comment). This if feels is the most viable route.

Remove lite functions from all, document them manually in the docstrings of their main functions (possibly with a decorator that automatically inserts the lite function docstring at the end of the main function).

Pros: Cleaner documentation (for the main functions)

Cons: Harder to find documentation on lite functions? Sphinx might complain about non-private functions being left out of all?

As I stated here, this is not an option. Not including <func>_lite in __all__ means <func>_lite would never be documented and, thus, the lite functionality would never be documented. It is absolutely necessary to include it.

StanczakDominik · 2021-04-03T05:39:22Z

Well, given #1100, let's definitely go with the automodsumm one! I would like to try my hand at adapting the solution from there to the _lite functions case - I guess the whole of #1099 resulted from me not knowing enough sphinx and I'd like to fix that! So I'd be completely fine with leaving them in the main table for now, with the understanding that I'll fix them later.

pheuer · 2021-04-03T13:50:11Z

<func>_lite is part of the public api and thus should be documented.

I definitely agree with this! My only concern is that I think having one very long list of functions in the documentation makes the package less approachable, especially to students new to plasma physics.

"Who are these crazy people and why do they have SO MANY DIFFERENT FREQUENCIES? Maybe I should transfer to a condensed matter group..."

I agree the automodsum implementation a la #1100 is the ideal way of handling this. I'd like to see a list of "Functions", followed by "Lite Functions" and "Aliases" (in whatever order). Then the first thing the user sees is a more manageable list of the main functions, and they have to scroll down to see the different versions of those functions.

rocco8773 · 2021-04-03T18:59:06Z

As I stated in #1100 (comment), I think I have a solution for grouping lite functions, so stay tuned for a prototype.

As I stated before, I think the lite functions should be document like everything else and I do agree they cause clutter on the documentation page. I like the ability to group them in a separate section that explains what they are.

rocco8773 added 20 commits March 28, 2021 11:59

add _ThermalSpeed class

b347074

move coefficients into the __call__ method and out of unitless

4182ac8

add example docstrings

1611bcd

add @njit to unitless method

706ce00

cleanup

18c4feb

add import of preserve_signature

1ce13ee

make coefficients a property

0d4762b

rename light -> lite

dd61a67

expand _ThermalSpeed docstring

0119993

missed rename of unitless -> lite

c092a35

remove autmodapi option :include-all-objects: since it can not proper…

5958963

…ly document imported attributes/variables

fix heading, add option :include-al-objects: to automodapi so defined…

658c031

… attributes/variables are documented

initial commit for the add_lite decorator

f502723

expose add_lite to the plasmapy.utils.deocorators namespace and add t…

8cbf2fd

…o __all__

clean imports and import add_lite

0795df2

rename _coefficients -> thermal_speed_coefficients

c37a5c1

initial commit for thermal_speed_lite

8293dab

delete _ThermalSpeed class; bind thermal_speed_lite to thermal_speed.…

a27b9ac

…lite using add_lite; incorporate thermal_speed_lite into the thermal_speed calculation

add thermal_speed_lite and thermal_speed_coefficients to __all__

729012f

update docstrings with specific reference; actually commit the add_li…

b64b879

…e decoration

rocco8773 requested review from pheuer and StanczakDominik April 2, 2021 03:57

github-actions bot added docs PlasmaPy Docs at http:https://docs.plasmapy.org plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.utils Related to the plasmapy.utils subpackage labels Apr 2, 2021

rocco8773 self-assigned this Apr 2, 2021

rocco8773 added this to In Progress | Draft in Formulary Development via automation Apr 2, 2021

rocco8773 added 2 commits April 1, 2021 21:03

appease grumpy black

785ae1f

put numba into the requirements

fd7aacd

appease grumpy black

f73c4ec

StanczakDominik previously approved these changes Apr 2, 2021

View reviewed changes

Formulary Development automation moved this from In Progress | Draft to In Progress | Review Apr 2, 2021

add numba to tox minimal deps

013b613

StanczakDominik mentioned this pull request Apr 2, 2021

Optimizing formulary functions #1085

Closed

pheuer requested changes Apr 2, 2021

View reviewed changes

pheuer mentioned this pull request Apr 2, 2021

Semi-dynamically generated aliases submodule #1099

Closed

4 tasks

StanczakDominik added the prototype 🏗️ Trying out an implementation on a trial basis label Apr 3, 2021

rocco8773 mentioned this pull request Apr 6, 2021

Add a LaTeX string attribute to formulary functions for standard symbols #1103

Open

rocco8773 added the plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage label Jun 30, 2021

rocco8773 dismissed StanczakDominik’s stale review via 013b613 February 26, 2022 21:19

namurphy added the status: dormant PRs that are stalled label May 26, 2023

namurphy added this to the Future milestone Apr 9, 2024

-"""
-Dictionary of various coefficients used in calculating the
-`~plasmapy.formulary.parameters.thermal_speed`.
-"""
+"""
+Dictionary of various coefficients used for
+`~plasmapy.formulary.parameters.thermal_speed`, with the dimensionality
+as the key.
+"""

		A lite weight version of `thermal_speed` intended for computational used
		and, thus, does not do any argument validation/conditioning.

	Lightwieght Version: `~plasmapy.formulary.parameters.thermal_speed_lite`
	Lightweight Version: `~plasmapy.formulary.parameters.thermal_speed_lite`

	coeff = thermal_speed_coefficients[ndim]
	coeffs_for_dim = thermal_speed_coefficients[ndim]

	wrapper_doc = inspect.cleandoc(getattr(wrapper, "__doc__", """"""))
	wrapper_doc = inspect.cleandoc(getattr(wrapper, "__doc__", ""))

[PROTO] Framework for lite "fast" formulary functionality #1098

Are you sure you want to change the base?

[PROTO] Framework for lite "fast" formulary functionality #1098

Conversation

rocco8773 commented Apr 2, 2021

StanczakDominik commented Apr 2, 2021

codecov bot commented Apr 2, 2021 • edited Loading

Codecov Report

StanczakDominik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheuer commented Apr 2, 2021 • edited Loading

StanczakDominik commented Apr 2, 2021 • edited Loading

rocco8773 commented Apr 2, 2021

rocco8773 commented Apr 2, 2021 • edited Loading

StanczakDominik commented Apr 3, 2021

pheuer commented Apr 3, 2021 • edited Loading

rocco8773 commented Apr 3, 2021

codecov bot commented Apr 2, 2021 •

edited

Loading

StanczakDominik Apr 2, 2021 •

edited

Loading

StanczakDominik Apr 2, 2021 •

edited

Loading

pheuer commented Apr 2, 2021 •

edited

Loading

StanczakDominik commented Apr 2, 2021 •

edited

Loading

rocco8773 commented Apr 2, 2021 •

edited

Loading

pheuer commented Apr 3, 2021 •

edited

Loading