Skip to content

Improved benchmark in solarposition.py #1443

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

roger-lcc
Copy link
Contributor

@roger-lcc roger-lcc commented Apr 14, 2022

  • Closes ASV benchmark Improvement #1441
  • I am familiar with the contributing guidelines
  • Adds description and name entries in the appropriate "what's new" file in 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`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Improved benchmark in solarposition.py
Add SolarPositionSlow and SolarPositionFast class. In SolarPositionSlow, test functions using ndays=[1, 10, 100] and times_localized. In SolarPositionFast, test functions using ndays=[1, 365 * 10, 365 * 100] and times_daily.

@kandersolar
Copy link
Member

Thanks @roger-lcc! Can you take a look at addressing the Stickler CI comments about code formatting?

@roger-lcc
Copy link
Contributor Author

Apologize for this!
I have addressed the stickler ci comments about the code formatting.
Thanks! @kanderso-nrel

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @roger-lcc this is looking pretty good. At the moment I'm not able to try this out locally, but I will do that later today.

def time_ephemeris(self, ndays):
solarposition.ephemeris(self.times, self.lat, self.lon)
# def time_ephemeris(self, ndays):
# solarposition.ephemeris(self.times, self.lat, self.lon)
Copy link
Member

Choose a reason for hiding this comment

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

How come these lines are commented out?


def time_get_solarposition(self, ndays):
solarposition.get_solarposition(self.times_localized,
self.lat, self.lon)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure there's any need to benchmark get_solarposition -- since it's just a convenience wrapper around the various solar position algorithms, this is more or less a duplicate of the time_spa_python benchmark.

@kandersolar kandersolar added this to the 0.9.2 milestone Apr 18, 2022
@roger-lcc
Copy link
Contributor Author

Is there something changed in Test_conda_linux Python ? The code checks were not successful.

@kandersolar
Copy link
Member

Ah, sorry for the confusion. It's okay to ignore the Test checks for this PR. What's happening is that pvlib has some functions to make internet requests to external servers and our tests fail when those external servers have issues. So those failures are not related to this PR. Since this PR isn't changing any of the actual pvlib package code, really the only check relevant here is the Stickler formatting check.

@roger-lcc
Copy link
Contributor Author

roger-lcc commented Apr 19, 2022

Thanks for your explanation! Now I have a clear understanding of the situation. Thanks again!

@kandersolar kandersolar mentioned this pull request Jul 26, 2022
9 tasks
@kandersolar kandersolar modified the milestones: 0.9.2, 0.9.3 Aug 19, 2022
@kandersolar kandersolar modified the milestones: 0.9.3, 0.9.4 Sep 14, 2022
@kandersolar kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@kandersolar kandersolar modified the milestones: 0.9.4, 0.9.5 Dec 14, 2022
@kandersolar kandersolar removed this from the 0.9.5 milestone Mar 18, 2023
@kandersolar kandersolar added this to the 0.9.6 milestone Mar 18, 2023
@Lakshyadevelops
Copy link
Contributor

Why was this not merged and being delayed to further versions are there any issues?

@kandersolar
Copy link
Member

Why was this not merged and being delayed to further versions are there any issues?

I think I recall wanting to make some changes to the classification of "slow" versus "fast", but it's been long enough that I forget the details. I need to take another look. The intent is to merge this PR eventually, it just hasn't been a priority.

@kandersolar kandersolar modified the milestones: 0.9.6, 0.10.0, Someday May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASV benchmark Improvement
4 participants