-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
There was a problem hiding this 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.) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
👍
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`) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🚀
There was a problem hiding this 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 🎉
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.