pvanalytics
pvanalytics copied to clipboard
pvlib
Implement Lorenz et al. POA and GHI QC methods
- [x] Closes #123
- [x] Added tests to cover all new or modified code.
- [x] Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
- [x] Added new API functions to
docs/api.rst. - [x] Non-API functions clearly documented with docstrings or comments as necessary.
- [x] Adds description and name entries in the appropriate "what's new" file
in
docs/whatsnewfor 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`). - [x] Pull request is nearly complete and ready for detailed review.
- [x] Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.
This is a draft PR. I need help editing the .rst files.
The original paper mentions that the proposed limits are specifically for 25° and SI pyranometers. While they are probably useful for other cases too, I think it would be worth adding a comment concerning this in a "Notes" section.
How does one avoid this?
I need to refer this link a couple of times, but in doing so, I am ending up with this unclean format. (Having the same issue in the functions in irradiance.py
As noted in the Notes section: "The upper limit for ghi is set to 0 when solar_zenith is greater than 90 degrees." Similarly, the lower limit is also 0 when the zenith angle is greater than 75 degrees. Thus, at nighttime, any value that is not exactly zero is flagged which I think is undesirable (irradiance measurements at nighttime typically have a slightly negative value, which is often accounted for by other QC methods). I don't think the authors meant for these checks to be applied for nighttime, but I suggest that we reach out and ask them.
Also, I don't like bundling in the max. step function. I could easily see myself using a custom mixture of upper and lower boundaries and setting other values for the max. step. Thus, I suggest having a separate max. step function for increased flexibility (the max step function was first proposed by Espinar et al. (2011) as noted in the paper).
How does one avoid this?
There's no way that I'm aware of. If you want a laugh, see what it does to poor read_tmy3. Anyway don't worry about it here.
As noted in the Notes section: "The upper limit for ghi is set to 0 when solar_zenith is greater than 90 degrees." Similarly, the lower limit is also 0 when the zenith angle is greater than 75 degrees. Thus, at nighttime, any value that is not exactly zero is flagged which I think is undesirable (irradiance measurements at nighttime typically have a slightly negative value, which is often accounted for by other QC methods). I don't think the authors meant for these checks to be applied for nighttime, but I suggest that we reach out and ask them.
Also, I don't like bundling in the max. step function. I could easily see myself using a custom mixture of upper and lower boundaries and setting other values for the max. step. Thus, I suggest having a separate max. step function for increased flexibility (the max step function was first proposed by Espinar et al. (2011) as noted in the paper).
On the first point: I agree with the fact that real irradiance measurements at nighttime rarely are 0. Does somebody watching this PR know any of the authors? If not, I don't mind reaching out to them.
On the second point: Yes, I think it would be better to separate the max step function, along with keeping the step value as an input (with $1000 W/m^2$ as default). The only thing against this change is that it deviates from the paper's criteria, which groups lower, upper and max step together. A middle way (which still deviates from the paper's criteria) is that we can keep the max step value user-defined, which would allow one to circumvent that particular filter by just using a really high value. However, I am completely fine with separating out the filter as a separate function :relaxed:.
I know Elke and can reach out. I'll cc you if you send your email. If anyone else wants to be cc'ed let me know.
I can dig the idea of the user specified max step - smart! Having it (also) be a separate function might still be better, but I'm interested in other's people's opinions.
I think that flagging low irradiance at nighttime is OK in the context of pvanalytics. The package provides other functions which label day/night periods, and each quality function returns a time series of Boolean for the critieria it checks. The temptation is to regard 'True' here as equivalent to a valid measurement - and that is not always the case, as the discussion brings out.
irradiance measurements at nighttime typically have a slightly negative value
That is certainly true for thermopile instruments, but not for all instruments. I think a reference cell would report a small positive value.
Elke Lorenz has written the following to me:
For the naming: Do you think it would be o.k. to name the functions “pvlive” instead of “Lorenz”? I think that using the name of the main author of a paper is what you typically do, but this was really team work and so I think “pvlive” would be more appropriate.
Thus, I suggest that we change the name as requested. If anyone have any objection, please comment!
Sounds good!
Abhi
On Mon, Mar 20, 2023, 10:41 AM Adam R. Jensen @.***> wrote:
Elke Lorenz has written the following to me:
For the naming: Do you think it would be o.k. to name the functions “pvlive” instead of “Lorenz”? I think that using the name of the main author of a paper is what you typically do, but this was really team work and so I think “pvlive” would be more appropriate.
Thus, I suggest that we change the name as requested.
— Reply to this email directly, view it on GitHub https://github.com/pvlib/pvanalytics/pull/167#issuecomment-1476669851, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG6HX5ENAP55SUS7R3Y3BN3W5CJDBANCNFSM6AAAAAAR6UZCRI . You are receiving this because you were mentioned.Message ID: @.***>
The code seems good, so well done!
My main concern is that it seems like an awful lot of code to implement the integer flags. I would prefer to just have the individual functions, but it's not a strong preference.
Do you have any recommendations? For e.g. would you combine the upper and lower limit functions? Individual functions provide clarity, but I am okay with combining them.
The code seems good, so well done!
My main concern is that it seems like an awful lot of code to implement the integer flags. I would prefer to just have the individual functions, but it's not a strong preference.
Yeah, I agree, the code is definitely longer for what it's doing.
@abhisheksparikh I think we should explain the integer flag values. I don't know if my suggestions will render properly as a web page.
The authors have responded to our question concerning what to do for nighttime values:
Regarding the night values: in our implementation we set all night values to zero and set a Flag with value 1 for all values changed. But actually, the method is not made for night values. So as you wrote, not checking is also an option which is maybe better for general applications. For some applications night values might be of interest, we actually do not use them anywhere.
So for the integer output, I think we should set nighttime values to 1 ~~True~~. But for the bool series returned, I suggest not setting any nighttime limits but rather setting a flag of True or False depending on the outcome of #191.
Additionally, the authors also wrote:
One other thing I have to point out. While checking your implementation we found a mistake in the publication, which is very unfortunate. We write I0 as solar constant with a fixed value. But actually I0 should be the Normal extraterrestrial irradiance corrected from Sun-Earth distance, which has a yearly cycle. We use the formula proposed by Spencer 1971 with a simplification suggested in “Energy Meteorology, Lecture Notes from Detlev Heinemann, 2002 Formula 2.16”
@abhisheksparikh This should be relatively straightforward to correct. You can look at the existing tests which do something very similar.
So for the integer output, I think we should set nighttime values to True. But for the bool series returned, I suggest not setting any nighttime limits but rather setting a flag of True or False depending on the outcome of https://github.com/pvlib/pvanalytics/issues/191.
@AdamRJensen - I am confused by this: 'So for the integer output, I think we should set nighttime values to True'.
I will change the Normal extraterrestrial irradiance based on the formula.
@AdamRJensen - I am confused by this: 'So for the integer output, I think we should set nighttime values to True'.
A typo on my behalf - the flag for the integer should of course be 1 as stated by the authors 😄
@antondriesse I wanted to bring your attention to this QC check for GTI.