Skip to content

ASV benchmark Improvement #1441

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

Closed
roger-lcc opened this issue Apr 12, 2022 · 7 comments · May be fixed by #1443
Closed

ASV benchmark Improvement #1441

roger-lcc opened this issue Apr 12, 2022 · 7 comments · May be fixed by #1443

Comments

@roger-lcc
Copy link
Contributor

roger-lcc commented Apr 12, 2022

Is your feature request related to a problem? Please describe.
In the existing benchmark, e.g solarposition.py ,I found that there are some functions which are not covered by benchmark. So I think should we cover more functions in each benchmark?

Describe the solution you'd like
cover more functions in each benchmark

Additional context
Please assign me this task
Thanks @kanderso-nrel

@kandersolar
Copy link
Member

Sure, I think probably most/all of the existing benchmarks don't 100% cover their corresponding code modules.

I think we certainly want to cover more functions, but I'm not sure we need to go all the way to 100%. For example pvlib.solarposition.spa_c requires an external file that is difficult to use in automated tests, so we probably don't need a benchmark for that function. Maybe it makes sense to make a list of functions that aren't currently covered, then figure out the subset that we want to cover first?

@roger-lcc
Copy link
Contributor Author

YEP!I also found that pvlib.solarposition.spa_c couldn't be automated tested because of the license of it. So I will figure out the subset that we want to cover first.
Thanks!

@roger-lcc
Copy link
Contributor Author

roger-lcc commented Apr 13, 2022

The subset in solarposition.py has been identified initially. And I have tested the subset functions by benchmark. And I found that the results of some functions in solarposition.py has the almost same time although we tried to run a single benchmark for multiple values of some parameter.
Should we move these functions to another class ? And we will test these functions only one time.
Thanks!@kanderso-nrel

@kandersolar
Copy link
Member

I guess you're talking about a result like this one where there's not a significant increase in runtime as ndays grows from 1 to 100?

Should we move these functions to another class ?

Hmm, maybe. It's definitely true that some functions in solarposition.py are much simpler and therefore faster than others. I think the problem isn't that those simple functions don't get slower with larger inputs, it's more that we aren't giving them inputs large enough to make the slowdown apparent. So it might make sense to have a "slow" class and a "fast" class, where the "slow" class keeps the current ndays=[1, 10, 100] and the "fast" class uses larger values like [1, 1000, 10000] (or whatever is appropriate).

@mikofski
Copy link
Member

  • 5-minute data for a year is 105,120 timesteps
  • 1-minute is 525,600
  • 25 years of 5-minute data would be 2,628,000
  • 25 years of 1-minute data would be 13,140,000

@Lakshyadevelops
Copy link
Contributor

The subset in solarposition.py has been identified initially.

Could you share that subset. Is it the functions implemented in #1443 ?
What all criteria are you using to determine whether a function has to benchmarked or not?

@roger-lcc
Copy link
Contributor Author

The subset in solarposition.py has been identified initially.

Could you share that subset. Is it the functions implemented in #1443 ? What all criteria are you using to determine whether a function has to benchmarked or not?

Sry, it's a long time that I don't focus on this project. I have forgotten the details. I think you can read the origin code. It's helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants