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

Updates for pvlib 0.8.0 #116

Merged
merged 3 commits into from
Mar 24, 2021
Merged

Updates for pvlib 0.8.0 #116

merged 3 commits into from
Mar 24, 2021

Conversation

kandersolar
Copy link
Contributor

@kandersolar kandersolar commented Mar 20, 2021

Closes #114

Turned out to be easier than I expected; only one change was needed (pvsystem.sapm_aoi_loss -> iam.sapm).

Tests pass for me locally for each of pvlib==[0.7.0, 0.7.1, 0.7.2, 0.8.0, 0.8.1, 0.9.0a6]. Not sure when 0.9.0 is coming out, so I think to be safe better keep <0.9.0 for now. Note that running the test suite did spawn several plot windows that I wasn't sure how to check.

Would it be helpful to create a v1.6.0 (?) whatsnew file in this PR, or do you want to do that separately?

Edit: maybe CircleCI isn't running because I'm opening this from an untrusted fork? I'm not familiar with CircleCI but I've seen that with other CI providers so that random people can't open a PR and access sensitive info with print(secret_api_key) in a test function. Let me know if there's something I can do on my end to get the CI to check this.

Copy link
Contributor

@anomam anomam left a comment

Choose a reason for hiding this comment

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

Thanks a ton @kanderso-nrel !
Yes, could you please add a new whatsnew file with your contribution? I tried to tweak the circleci config, and a new commit in this PR will hopefully trigger the CI build

@@ -681,6 +681,6 @@ def fn(angles):
# Transform the inputs for the SAPM function
angles = np.where(angles >= 90, angles - 90, 90. - angles)
# Use pvlib sapm aoi loss method
return pvlib.pvsystem.sapm_aoi_loss(angles, pvmodule, upper=1.)
return pvlib.iam.sapm(angles, pvmodule, upper=1.)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -1,4 +1,4 @@
pvlib>=0.6.0,<0.8.0
pvlib>=0.7.0,<0.9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @kanderso-nrel !
In my head, the upper bound I put in in the latest patch ("<0.8.0") was kind of temporary until I would find time to implement the fixes and bump the pvlib version. My original philosophy was to always try to use the latest version of pvlib, and let new versions break the tests so that I would know if there's an update needed in pvfactors.
But since I have very limited time for this project lately and not many contributions or messages (thanks again for yours!), I think that for now we can keep it, and if it causes any issue we will hear about it very soon.
👍

@kandersolar
Copy link
Contributor Author

Thanks @anomam! Looks like the CI is working. I've never been clear on whether semantic versioning would have dependency updates prompt a minor release or if just a patch release is sufficient -- I put the whatsnew entry under v1.5.1, but am happy to move to 1.6.0 or something else if you prefer.

v1.5.1 (DATE XX, 2021)
======================

Update pvlib dependency from ``pvlib>=0.6.0,<0.8.0`` to ``pvlib>=0.7.0,<0.9.0`` (:ghpull:`116`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ghpull role is provided by the sphinxcontrib_github_alt extension, but previous whatsnews just use plaintext #116 so I'll change this to plaintext for consistency if you want

Copy link
Contributor

Choose a reason for hiding this comment

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

super cool! I didn't know about this 🚀

Copy link
Contributor

@anomam anomam left a comment

Choose a reason for hiding this comment

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

@kanderso-nrel looking great! Thanks for the contribution 🎉

@anomam anomam merged commit 17bee34 into SunPower:master Mar 24, 2021
@kandersolar kandersolar deleted the pvlib_080 branch March 24, 2021 14:31
@anomam anomam mentioned this pull request Mar 27, 2021
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.

Update pvlib dependency to v0.8.0
2 participants