Skip to content

Improve some thermo calculation bottlenecks by refactoring how units are handled#2064

Merged
dopplershift merged 5 commits intoUnidata:mainfrom
jthielen:refactor-unit-handling-bottlenecks
Jan 21, 2022
Merged

Improve some thermo calculation bottlenecks by refactoring how units are handled#2064
dopplershift merged 5 commits intoUnidata:mainfrom
jthielen:refactor-unit-handling-bottlenecks

Conversation

@jthielen
Copy link
Collaborator

Description Of Changes

I noticed that there have been two major performance bottlenecks caused by the existing approaches to unit handling: iteration in moist_lapse/lcl (#1169) and looping of thickness_hydrostatic in io.gempak's _interp_* functions (#2062). In an attempt to improve this situation, this PR takes the approach of refactoring the relevant thermo calculations to have private versions that only use base units (no pint.Quantity handling), and wrapping those while handling units. No robust benchmarks were evaluated, but this did speed up pytest tests/calc/test_thermo.py on my workstation from about 3.5 seconds to 0.69 seconds (similar to #1980), and @akrherz's test file in #2041 (comment) from 35.88 seconds to 1.78 seconds.

I tried to be careful to not touch any tests in this refactor (other than the fix to the flake8 checker), but do let me know if any tests should be added.

Future work building on this PR could examine if all/most of the calculations should be refactored in this way (which may also reveal some cleaner implementation approaches) and if numba jit/guvectorize could meaningfully accelerate any of these private routines.

@sgdecker, could you evaluate how much performance improvement this gives you relative to your earlier tests in #2062?

@nawendt, these changes caused some failing tests in the GEMPAK reader due to what looks to be marginally different output values. Would you be willing to take a look and see if these differences are significant or not?

Checklist

@jthielen jthielen requested a review from a team as a code owner August 28, 2021 21:13
@jthielen jthielen added Area: Calc Pertains to calculations Area: IO Pertains to reading data Area: Units Pertains to unit information Type: Maintenance Updates and clean ups (but not wrong) labels Aug 28, 2021
@jthielen jthielen added this to the 1.2.0 milestone Aug 28, 2021
@nawendt
Copy link
Contributor

nawendt commented Aug 29, 2021

I added a comment to convert_degC_to_K. This function is the reason the tests started failing. It did not account for missing values which were most likely -9999. The reference data has a nan value whereas the decoded soundings ended up having some nonsense values in the same array position. Glad it was an easy fix.

@sgdecker
Copy link
Contributor

On an old laptop, I am going from 3:00 to 0:13 with this branch and my test file. Very nice!

@jthielen
Copy link
Collaborator Author

jthielen commented Aug 31, 2021

I wasn't all that happy with the private routine + public wrapper approach...it would get really messy if applied to the whole calculation suite. So, I went back and came up with a decorator-based alternative: https://gist.github.com/jthielen/93ec9fcd8f3892a83215309fe5cda2c6. Doing it this way moves all unit specification into the decorator (which does carry with it the slight issue of default arguments in the signature needing to be in base units and not Quanitities), and leaves the unitless version attached to the parent function on the ._nounit attribute (for use internally).

The existing approach is passing all checks, but let me know if it'd be useful and go back and redo it in the decorator-based way. If going that route, I'd want to clean it up to allow the existing check_units and this new process_units to better share code, and to think through if I'm missing any edge cases in the design. Otherwise, if we want to get these performance improvements in as soon as possible, the decorator alternative can be left to a future refactor when we need to optimize more routines.

@jthielen jthielen force-pushed the refactor-unit-handling-bottlenecks branch 2 times, most recently from 01592ce to 25b3567 Compare September 17, 2021 01:36
@jthielen
Copy link
Collaborator Author

This has now all been rewritten to use the decorator-based approach! Performance improvements from my tests are about the same as before.

@jthielen jthielen force-pushed the refactor-unit-handling-bottlenecks branch from 17d124c to 00745ba Compare September 19, 2021 19:28
@jthielen
Copy link
Collaborator Author

This should be ready for final review once it is adapted after the merge of #2263

@jthielen jthielen force-pushed the refactor-unit-handling-bottlenecks branch from 00745ba to 3289182 Compare January 18, 2022 19:23
@jthielen
Copy link
Collaborator Author

@dcamron I've rebased this on main now that #2263 is in. I'm not on my primary workstation at the moment, so wasn't able to check full lint, test suite, and performance, but at least we can see what CI thinks. Also, should be good enough for a review.

@dopplershift
Copy link
Member

We can ignore Code Climate.

dopplershift
dopplershift previously approved these changes Jan 21, 2022
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wish we had some actual benchmarks in place, but alas.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wish we had a benchmark, but alas.

@dopplershift dopplershift merged commit 94ef0e0 into Unidata:main Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Calc Pertains to calculations Area: IO Pertains to reading data Area: Units Pertains to unit information Type: Maintenance Updates and clean ups (but not wrong)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

snxarray is slow

4 participants