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

Pv prod fix #326

Merged
merged 12 commits into from
Jun 16, 2022
Merged

Pv prod fix #326

merged 12 commits into from
Jun 16, 2022

Conversation

adfarth
Copy link
Collaborator

@adfarth adfarth commented Jun 8, 2022

Please check if the PR fulfills these requirements

  • CHANGELOG.md is updated
  • Tests for the changes have been added (for bug fixes / features)
  • [N/A ] Docs have been added / updated (for bug fixes / features) --> @NLaws do you think we should update the user manual for this?

What kind of change does this PR introduce?

(Bug fix, feature, docs update, ...)

  • In src/pvwatts.py, Updated lat-long coordinates if-statement used to determine whether the "nsrdb" dataset should be used to determine the PV prod factor. Accounts for recent updates to NSRDB data used by PVWatts (v6)
  • Avoids overwriting user-entered PV azimuth (other than 180) for ground-mount systems in southern hemisphere
  • Updates default azimuth to 0 for southern latitudes for all PV types (rather than just for ground-mount)

What is the current behavior?

(You can also link to an open issue here)

  • Incorrect bounds of NSRDB dataset are applied in PVWatts API call, meaning we are using lower quality PV resource data for some international sites
  • Default azimuth remains at 180 for rooftop systems in southern hem
  • Overwrites user-entered PV azimuth for ground-mount systems in southern hemisphere

What is the new behavior (if this is a feature change)?

See changes

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
Yes? More accurate PV data will be used for international evaluations.

Other information:

These are the bounds that are now applied to the NSRDB query:
image

PVWatts v6 uses 1.2 default (this is also the default in nested_inputs)
were previously overwriting user-entered azimuth value for ground-mount systems in southern hemisphere. Now only overwritten if 180. Were previously not setting azimuth to 0 for roofmount systems. Now we are if azimuth =180
@adfarth adfarth requested a review from NLaws June 8, 2022 21:57
@adfarth adfarth marked this pull request as ready for review June 13, 2022 20:26
reo/src/pvwatts.py Outdated Show resolved Hide resolved
Copy link
Member

@NLaws NLaws left a comment

Choose a reason for hiding this comment

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

A couple comments to address but looks good!

Can we combine your new test with this old one:
https://github.com/NREL/REopt_API/blob/master/reo/tests/test_southern_hemisphere_latitude.py
?
Combining the two would make one less API post/optimization in the test suite.

@adfarth
Copy link
Collaborator Author

adfarth commented Jun 15, 2022

A couple comments to address but looks good!

Can we combine your new test with this old one: https://github.com/NREL/REopt_API/blob/master/reo/tests/test_southern_hemisphere_latitude.py ? Combining the two would make one less API post/optimization in the test suite.

Combined these tests here! 9eb58aa

Copy link
Member

@NLaws NLaws left a comment

Choose a reason for hiding this comment

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

Please merge if tests pass. We can also merge this into master after testing the develop deploy and merge to staging/production thereafter.

@adfarth adfarth merged commit 887a879 into develop Jun 16, 2022
@adfarth adfarth deleted the pv-prod-fix branch June 16, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants