-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Convert SPA native python loops to numpy operations #2537
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
base: main
Are you sure you want to change the base?
Conversation
@dfulu is this ready for the CI? |
Hi @cwhanse, yes it is |
@cwhanse I think the test failure on windows + python 3.9 is unrelated to the changes I've made. I'm not sure how to proceed |
@dfulu the py39 failure is unrelated. The portions of code activated by USE_NUMBA are shown as untested. That looks like something that needs fixed on pvlib's side. I'm not the right person to determine what that is. Although marked as successful, the benchmark report indicates that NUMBA couldn't be imported in the py39 environment for the speed checks. |
@cwhanse Thanks for the reply, I'll maybe wait until someone else is available to look. In the mean time I'll see if there is another way instead of using the |
Nice @dfulu! Have you checked how the vectorized version affects peak memory usage?
Thanks for noticing this. It's an issue on
Code coverage isn't (can't be) recorded for code that gets run via numba. There's no workaround as far as I'm aware, but the last time I checked was a couple years ago now. |
@kandersolar, I didn't think of that actually. I've had a look, and the vectorised version definitely uses more memory. I did a bit of testing of the I've also done a bit more profiling and found that this version only beats the main branch up to the order of 1000 input times, then they converge. It also only beats the numba version at around O(10) input times which is exactly the order I was testing at. So not as exciting as it first seemed |
Hi,
I'd like to make this proposal to speed up the
"nrel_numpy"
version ofpvlib.solarposition.get_solarposition()
In this PR I just replace some of the native python loops with numpy equivalent slicing and sums. In my local testing (using order 10 input times) I have found that this version take about 45% less time than the current version. It is also takes about 20% less time than the current
"nrel_numba"
version.When I modified the
sum_mult_cos_add_mult()
andlongitude_obliquity_nutation()
functions could no longer infer the datatypes of these functions. Hence these functions now use the globalUSE_NUMBA
setting. This allows numba to infer the dataypes whilst allowing the numpy version to avoid the native python loops.docs/sphinx/source/reference
for API changes.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.