Handle units external to iteration in moist_lapse and lcl#1980
Handle units external to iteration in moist_lapse and lcl#1980jthielen wants to merge 2 commits intoUnidata:mainfrom
Conversation
|
Test failures are due to human-imperceptible changes in the plotted moist adiabats. The repeated code really hurts me, but the benefits are really great. I'm willing to overlook it so long as we open an issue to remind us to do something about it--I'd say just refactor the guts of these calculations into some re-used private helper, but we can wait on that for a bit while we wait for the other things mentioned to shake out. Just need to fix up some of the naming lint. Also, is there any way we can avoid the disastrous equation formatting by black there? |
src/metpy/calc/thermo.py
Outdated
| exporter = Exporter(globals()) | ||
|
|
||
| sat_pressure_0c = units.Quantity(6.112, 'millibar') | ||
| sat_pressure_0c_in_base_units = 611.2 |
There was a problem hiding this comment.
Why not use to_base_units().m with the value right above?
src/metpy/calc/thermo.py
Outdated
| sat_pressure_0c_in_base_units = 611.2 | ||
| epsilon_in_base_units = mpconsts.epsilon.to_base_units().m | ||
| Rd_in_base_units = mpconsts.Rd.to_base_units().m | ||
| Lv_in_base_units = mpconsts.Lv.to_base_units().m |
There was a problem hiding this comment.
Could we go with something like base_unit_lv? mumbles something about stupid bots
src/metpy/calc/thermo.py
Outdated
| partial_press = ( | ||
| sat_pressure_0c_in_base_units | ||
| * np.exp(17.67 * (t - 273.15) / (t - 29.65)) | ||
| ) | ||
| rs = ( | ||
| epsilon_in_base_units * partial_press | ||
| / (p - partial_press) | ||
| ) | ||
| frac = ( | ||
| (Rd_in_base_units * t + Lv_in_base_units * rs) |
There was a problem hiding this comment.
Though this looks less awful without all the comments from the linter interspersed, so maybe it's not as bad.
640029e to
81232b3
Compare
|
@dopplershift I updated the base unit constant names, so hopefully that takes care of the bot complaints. It also resulted in shorter names which improved the line wrapping situation. That being said, the formatting was me just instinctively applying black-style wrapping rules, so after the variable rename, I redid the wrapping. Hopefully what is there now is more acceptable? w.r.t. the test failures, should I regenerate the test image or lower the tolerance? Also, that sounds good about an eventual refactor to re-used private helpers. |
|
Also, just because I didn't want to spend too much time working around it, I tweaked the flake8 check to ignore |
d65d0c2 to
41d1ccd
Compare
41d1ccd to
b661a93
Compare
|
Superseded by #2064. |
Description Of Changes
In the process of reviewing #1968, I began experimenting with performance optimizations to
moist_lapseto compare against the current implementation and the lookup table approach used there. While my ideal solution of usingguvectorizewithnumbacame up short for now (need an ODE solver that works withinnumba'snopythonmode, so something likeNumbaLSODAthat brings with it dependency troubles,numbakit-odethat's partially held up by some upstream issues, or a home-spun solver with increased maintenance costs), I was able to get around an order of magnitude improvement by moving unit handling outside of the iterator function, as done here.Since I went ahead and applied analogous changes to
lcl, this PR can replace the draft implementation of #1169.No robust benchmarks were evaluated, but this did speed up
pytest tests/calc/test_thermo.pyon my workstation from about 3.5 seconds to 0.68 seconds.The biggest caveat is that this moves the full chain of operations internal to these iterating functions (so any updates to the underlying functions will also have to modify the code here). However, I don't see that as much of a concern, since I'd expect the future
plug-gable calculation suite/automated solver"numbafied" internal routines to necessitate refactoring all of that anyways (EDIT: after some later analysis, I doubt that we'll be able to make a robustly plug-able suite while also using numba-complied underlying routines...but that doesn't change the fact that we'll likely be refactoring these internals relatively soon anyways).Checklist
Tests added(internal refactor only)Fully documented(internal refactor only)