Skip to content

Addressing unequal times error from pvlib.temperature.prilliman with no leap day #1476

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
williamhobbs opened this issue Jun 20, 2022 · 5 comments · Fixed by #1490
Closed
Milestone

Comments

@williamhobbs
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When using pvlib.temperature.prilliman(), if the weather data does not include leap day (e.g., using 2020 PSM3 from pvlib.iotools.get_psm3() with the default leap_day=False), it returns:

NotImplementedError: algorithm does not yet support unequal times. consider resampling your data.

This could be a common scenario and could be confusing to the user.

Describe the solution you'd like
There are several possible solutions:

  1. Modify the error text to always say something like NotImplementedError: algorithm does not yet support unequal times. consider resampling your data. check to make sure your data includes leap day for leap year(s).
  2. Check to see if leap day is missing and tailor the error message accordingly (e.g., add leap day is missing from your data. to the error message).
  3. Check to see if leap day is missing, automatically fill it in somehow (e.g., repeat Feb 28), then remove it from the results before returning the output time series.
  4. Same as 3, but also notify the user that this has happened. Depending on time zone of the weather data, there might be a discontinuity in the resulting smoothed temperature after removing leap day (I think?).

Describe alternatives you've considered
Manually fill in leap day or repeat weather data query with leap day included (e.g., use leap_day=True with pvlib.iotools.get_psm3()).

@adriesse
Copy link
Member

Interesting problem. The missing leap day can cause problems in other functions as well. I would suggest that the best practice would be for the user to insert the leap day into their DataFrame using a method appropriate for their situation.

Perhaps the default should be changed to leap_day=True?

@williamhobbs
Copy link
Contributor Author

@adriesse Agreed. That's the solution I used - repeat my psm3 weather query with leap_day=True. But I do think that the error messaging for pvlib.temperature.prilliman() could better help users troubleshoot this issue.

Separately, I also like the idea of changing pvlib.iotools.get_psm3() so that the default is leap_day=True. Should that suggestion be opened as a separate issue? 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.

@AdamRJensen
Copy link
Member

@williamhobbs in regards to:

Separately, I also like the idea of changing pvlib.iotools.get_psm3() so that the default is leap_day=True. Should that suggestion be opened as a separate issue? 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.

Then I recommend that is made a separate issue.

@kandersolar
Copy link
Member

Checking for missing leap days and trying to correct for them seems out of scope for temperature.prilliman to me. I agree with @adriesse, better to leave that can of worms to the user. But I am certainly in favor of improving that error message to mention leap days and perhaps data gaps in general. Maybe something like this?

algorithm does not yet support unequal times. consider resampling your data and checking for jumps from missing periods, leap days, etc.

We might also want to change "unequal times" to something else while we're at it. Note that this same error message is used in detect_clearsky, so we should make sure the message continues to be appropriate for both functions.

As an aside, I suspect leap_day=False is rooted in matching the PSM3 API's decision to set it False by default. But +1 to a new issue to discuss changing it on the pvlib side.

@AdamRJensen
Copy link
Member

It may also be worth updating the warning admonition to explicitly mention the issue with leap days.

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