-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve pvlib.spa
speed
#1748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve pvlib.spa
speed
#1748
Conversation
This reverts commit 75e6265.
Ready for review. The drop in reported coverage should be ignored; it's in the part of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @kandersolar!
@@ -468,60 +456,32 @@ def julian_ephemeris_millennium(julian_ephemeris_century): | |||
return jme | |||
|
|||
|
|||
@jcompile(nopython=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to declare types in this jcompile like we do in most of the other functions in this module? I haven't kept up with numba development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert here either, but I think specifying types is not necessary in general; when using numba for JIT compiling like we do, it can infer types as needed.
In this particular case, I did try to specify types just for consistency with the rest of the code. However it seems that specifying the appropriate type for module-level array constants is not possible using the shorthand string notation. It is possible by importing numba, but I didn't see a clean way to no-op the type declaration when numba isn't installed, so I opted to just leave the type signature empty here instead. I've added a comment along these lines (should have done that originally).
* :py:mod:`pvlib.spa`, and :py:func:`pvlib.solarposition.get_solarposition`/ | ||
:py:meth:`pvlib.location.Location.get_solarposition` with ``method='nrel_numpy'`` | ||
or ``method='nrel_numba'``, are now 50-100% faster than before. (:pull:`1748`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also pvlib.solarposition.spa_python
and ModelChain.run_model*
. Maybe less is more in this situation: The default solar position algorithm (NREL SPA) is now 50-100% faster.
Co-Authored-By: Will Holmgren <william.holmgren@gmail.com>
docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Some functions in
pvlib.spa
are doing quite a lot of unnecessary computation:longitude_nutation
andobliquity_nutation
functions perform the same expensive calculationnp.radians(x)
.This PR prevents those unnecessary computations and reduces overall calculation time by 40-50% for both numpy and numba. The table below compares runtimes in seconds:
Regarding the table-length issue, it might be better to do away with the combined
HELIO_LONG_TABLE
et al. entirely and just keep the arrays as separate module-level variables.