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

Implement Lorenz et al. POA and GHI QC methods #167

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

Conversation

abhisheksparikh
Copy link

@abhisheksparikh abhisheksparikh commented Nov 13, 2022

  • Closes QA methods for plane-of-array irradiance #123
  • Added tests to cover all new or modified code.
  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
  • Added new API functions to docs/api.rst.
  • Non-API functions clearly documented with docstrings or comments as necessary.
  • Adds description and name entries in the appropriate "what's new" file
    in docs/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.

This is a draft PR. I need help editing the .rst files.

@abhisheksparikh abhisheksparikh marked this pull request as ready for review November 16, 2022 07:48
@kperrynrel kperrynrel added the enhancement New feature or request label Nov 17, 2022
@kperrynrel kperrynrel added this to the v0.1.3 milestone Nov 17, 2022
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 @abhisheksparikh, this is looking pretty good. This PR has some merge conflicts, largely due to editing the same files as #163 did. If you need help resolving those let us know.

Also, the reference says:

Additionally, we limit the maximal step between two consecutive irradiance measurements to 1000 W/m2 for both GHI and GTI as proposed for also in Espinar et al. (2011).

Do we need to do that as well in order to claim we have fully implemented the method as published?

pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Outdated Show resolved Hide resolved
@kperrynrel kperrynrel self-requested a review November 28, 2022 20:05
@kandersolar kandersolar mentioned this pull request Dec 9, 2022
docs/whatsnew/0.1.3.rst Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Outdated Show resolved Hide resolved
pvanalytics/tests/quality/test_irradiance.py Show resolved Hide resolved
@kandersolar
Copy link
Member

Friendly ping @abhisheksparikh about this PR :)

docs/api.rst Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

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.

pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
Comment on lines +668 to +669
The upper limit for `poa_global` is set to 0 when `solar_zenith` is greater
than 90 degrees. Missing values of `poa_global`, `solar_zenith`
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 still in strong favor of not flagging values at nighttime. I recently had a chat with the main author of the paper who did not have a recommendation for night time. Lorenz pointed me toward one of the co-authors who has yet to respond to my request.

Copy link
Author

Choose a reason for hiding this comment

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

Not having thought about the significance of nighttime data, I don't have a strong opinion on this. But if I were to think of irradiance as a part of operational analysis of a PV plant, I would like to have some distinction between daytime and nighttime data.

Comment on lines +696 to +701
# Changing the poa_global_flag to 3 when the step change in poa values is
# more than 1000 W/m2
poa_global_limit_int_flag = poa_global_limit_int_flag.mask(
poa_global.diff().abs() > 1000,
3
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this deserves it's own function.

The step change limit is fairly common:

For 1-min data Shafer et al. (2000) recommended a step change limit of 800 W/m2, although Espinar (2011) suggested 1000 W/m2 for GHI. Lorenz et al. (2022) also suggested using the 1000 W/m2 threshold for tilted irradiance measurements.

Copy link
Member

@AdamRJensen AdamRJensen May 30, 2023

Choose a reason for hiding this comment

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

It could be done in a separate pull request.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I think one good reason to have the step change limit in here is to stay true to the list of checks the paper suggests.

Copy link
Member

Choose a reason for hiding this comment

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

we could have a helper function that returns the step threshold, and pass it a kwarg 'pvlive' to return 1000.

Copy link
Member

Choose a reason for hiding this comment

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

However, I am completely fine with separating out the filter as a separate function ☺️.

As am I, let's do this and have a step_limit kwarg or something.

Copy link
Author

Choose a reason for hiding this comment

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

we could have a helper function that returns the step threshold, and pass it a kwarg 'pvlive' to return 1000.

Can somebody help me with this? After some reading, I think I understand kwargs, but have never written a function with them. Some more guidance on how the functions' design would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

@abhisheksparikh let's do that in a follow-on pull request. I feel like we are good enough here and we can improve later. Agree @AdamRJensen ?

@abhisheksparikh
Copy link
Author

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.

@abhisheksparikh
Copy link
Author

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.

pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
pvanalytics/quality/irradiance.py Outdated Show resolved Hide resolved
@cwhanse
Copy link
Member

cwhanse commented May 30, 2023

@abhisheksparikh I think we should explain the integer flag values. I don't know if my suggestions will render properly as a web page.

@cwhanse cwhanse mentioned this pull request May 30, 2023
4 tasks
@AdamRJensen
Copy link
Member

AdamRJensen commented Jun 8, 2023

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.

@abhisheksparikh
Copy link
Author

abhisheksparikh commented Jun 14, 2023

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 #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
Copy link
Member

@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 😄

@kandersolar kandersolar modified the milestones: v0.2.0, v0.2.1 Feb 14, 2024
@kandersolar kandersolar modified the milestones: v0.2.1, v0.2.2 Oct 16, 2024
@AdamRJensen
Copy link
Member

AdamRJensen commented Oct 22, 2024

@adriesse I wanted to bring your attention to this QC check for GTI.

@adriesse
Copy link
Member

Thanks @AdamRJensen. We have read Lorenz 2022.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QA methods for plane-of-array irradiance
7 participants