Skip to content
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

Set leap_day=True as default in pvlib.iotools.get_psm3() #1481

Closed
williamhobbs opened this issue Jun 22, 2022 · 7 comments · Fixed by #1511 or #1991
Closed

Set leap_day=True as default in pvlib.iotools.get_psm3() #1481

williamhobbs opened this issue Jun 22, 2022 · 7 comments · Fixed by #1511 or #1991
Labels
Milestone

Comments

@williamhobbs
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Several functions require equal timesteps with no gaps (e.g., pvlib.temperature.prilliman and detect_clearsky). Having weather data that excludes leap day will cause an error. Specifically, using pvlib.iotools.get_psm3 for a leap year with the default leap_day=True would be a common reason for encountering this error.

Describe the solution you'd like
As suggested by @adriesse here #1476 (comment), perhaps the default should be changed to leap_day=True?

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Related to #1476.

On the topic of "is this a good idea", some comments from issue 1476:

"I wonder if there are any compelling reasons to keep it as False, other than the fact that other tools (e.g., NREL SAM) can't handle leap day."

(by me)

"I suspect leap_day=False is rooted in matching the PSM3 API's decision to set it False by default."

(by @kanderso-nrel)

@wholmgren
Copy link
Member

I want to make sure I understand the API... PSM3 ignores leap_day for tmy/tgy requests and always returns a year with 365 days? But defaults to skipping the leap day when requesting, say, 5 minute data for 2020? If I understand correctly, I agree that the default should change.

@williamhobbs
Copy link
Contributor Author

In regards to the NREL NSRDB API, there is no leap_day parameter for PSM3 typical years like there is for hourly PSM3 and 5min PSM3. I think it is safe to assume that the PSM3 TMY API will never return a leap day, even if February for the given TXY dataset happened to come from a leap year. But I've never seen that documented.

For pvlib.iotools.get_psm3, I'll defer to someone else to confirm, but I think you are correct on both points.

@kandersolar
Copy link
Member

The statements above about the behavior of the PSM3 API endpoints are consistent with my understanding 👍

@adriesse
Copy link
Member

A potential alternative would be not to have a default behavior in order to force users to make a conscious choice.

@AdamRJensen
Copy link
Member

I do not know of any other service that excludes leap days when requesting normal (non TMY) time-series data; hence, I think changing the get_psm3 to include leap days by default would be in line with the remaining iotools and the general expectations of users. I also prefer this to not having a default choice. After all real-world systems do indeed observe leap days!

@kandersolar kandersolar added the io label Jul 19, 2022
@kandersolar kandersolar added this to the 0.9.2 milestone Jul 19, 2022
@kandersolar
Copy link
Member

Any thoughts on whether this deserves a deprecation cycle like we've done for map_variables in #1374? I could go either way.

@AdamRJensen
Copy link
Member

I'm ok with no deprecation cycle. I suppose then it should first be implemented in 0.10 (change the milestone)?

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