Updates for pvlib 0.9.0 by kandersolar · Pull Request #121 · SunPower/pvfactors

kandersolar · 2021-09-14T21:35:27Z

Howdy @anomam, pvlib 0.9.0 was released the other day. No changes needed in pvfactors itself, but there is one change needed to the test suite related to a bugfix in the Perez model (pvlib/pvlib-python#1239). I have updated the values here to pass with pvlib 0.9.0, but that means the test fails on older pvlib versions. Is that acceptable? I could make the test values depend on the installed pvlib version if you want the tests to pass with older pvlibs as well. I suspect CircleCI will install the latest pvlib for this PR, but we'll see what actually happens.

I also took the liberty of creating a 1.5.2 whatsnew, although a note about different irradiance values w/ pvlib 0.9.0 may be in order.

Unrelated: may want to add python 3.9 to the test matrix -- 3.10 release candidates are already available on some CIs!

cc @spaneja

kandersolar · 2021-09-14T21:44:37Z

Pushed a sphinx fix (link) -- sorry for the scope creep, but I wanted to see what it thought of the test suite.

Edit: tests all pass, although I note that the 3.7 job decided to build pandas from source for some reason 🤔

anomam · 2021-09-15T12:49:47Z

Thank you so much for the contribution again @kanderso-nrel !
@pfranz-spwr @srisukhi @loganrozanski I do not have admin rights on the repo anymore and I can't merge this. I would be happy to keep maintaining pvfactors if you can give me elevated rights, or feel free to take over if you prefer. Please let us know what you prefer!

anomam

Thanks @kanderso-nrel ! LGTM

anomam · 2021-10-03T02:21:28Z

 ======================================

-In the "fast mode", ``pvfactors`` assumes that all incident irradiance values for the system are known except for the PV row back surfaces. So since the system to solve is now explicit (no matrix inversion needed), it runs a little bit faster than the full mode, but it is less acurrate.
+In the "fast mode", ``pvfactors`` assumes that all incident irradiance values for the system are known except for the PV row back surfaces. So since the system to solve is now explicit (no matrix inversion needed), it runs a little bit faster than the full mode, but it is less accurate.


anomam · 2021-10-03T02:21:37Z


 def setup(app):
-    app.add_stylesheet('css/custom.css')
+    app.add_css_file('css/custom.css')


anomam · 2021-10-03T02:22:05Z

+Contributors
+------------
+* Marc Anoma (:ghuser:`anomam`)
+* Kevin Anderson (:ghuser:`kanderso-nrel`)


anomam · 2021-10-03T02:22:16Z

    # Check expected outputs: before v1.3.0, expected output is
    # [20.4971271991293, 21.389095477613356], which shows discontinuity
-    expected_out = [20.497127, 20.50229]
+    expected_out = [20.936348, 20.942163]


Do we want to preserve the values for pvlib 0.8.0 in a comment, analogous to what was done above?

Good idea, done

anomam · 2021-10-03T02:22:41Z

@@ -1,4 +1,4 @@
-pvlib>=0.7.0,<0.9.0
+pvlib>=0.7.0,<0.10.0


@kanderso-nrel The thought popped into my head last night as to if it we should require pvlib 0.9.0 as a minimum, because of the error involved in the earlier versions.

(The error appears minor in the test you fixed here, but maybe that is a false impression and/or shouldn't matter.)

anomam · 2021-10-03T02:28:12Z

@pfranz-spwr @srisukhi @loganrozanski could you please merge this PR if it looks good to you?

srisukhi · 2021-10-03T02:32:10Z

Thanks for the heads up, @anomam.

@campanelli-sunpower can you take a look and merge if the PR looks good?

kandersolar · 2021-10-04T14:32:25Z

I do think a note somewhere about different results would be a good idea -- it's only a minor difference, but exact reproducibility is important in some contexts, and it might save some users some pain if they know to expect differences with pvlib 0.9.0+. Whether that note should go in the change log, or maybe a docstring, or maybe just the narrative docs, I'm not sure. Let me know if you want me to add something like that to this PR :)

Also howdy @campanelli-sunpower, nice to see another pvlib name here!

campanelli-sunpower · 2021-10-06T15:54:11Z

@kanderso-nrel (cc @anomam) Thanks for updating the comment for posterity, so that consumers will be aware of the presence of the error with earlier versions of pvlib. What are your thoughts about releasing this with a new lower limit of pvlib 0.9.0?

kandersolar · 2021-10-06T16:22:52Z

I don't feel strongly one way or the other. Requiring pvlib>=0.9.0 is a bit strict considering it was only released a month ago and is breaking wrt the 0.8.x series. But getting different results based on which version of pvlib happens to be installed is not desirable either. I would probably lean towards increasing the minimum to 0.9.0 to prioritize correctness over consistency with older results. But I am not a pvfactors user myself so I'll defer to others for that decision.

campanelli-sunpower · 2021-10-07T19:03:07Z

@anomam What are your thoughts on requiring pvlib>=0.9.0, in order to ensure users aren't affected by the known bug in earlier pvlib?

anomam · 2021-10-10T14:25:44Z

hey team @kanderso-nrel @campanelli-sunpower ! My preference would also be to set pvlib>=0.9.0. In my opinion we want pvfactors to be as accurate as possible, so if we know that there is a fix in a model, I think we should enforce its use even if it's inconsistent with previous versions.
Let's just make sure we put a note in the release notes! 🙏

kandersolar · 2021-10-11T15:05:49Z

Requirement updated and note added :)

campanelli-sunpower

Thanks for the update.

kandersolar added 5 commits September 14, 2021 15:08

change pvlib dep from <0.9.0 to <0.10.0

44506b6

unrelated typo

a9de617

update test expected values

5d783a6

create 1.5.2 whatsnew

a9c251f

update deprecated sphinx code

4ca5d5d

kandersolar mentioned this pull request Sep 15, 2021

Update documented python version support; add py3.9 test configuration #122

Merged

anomam approved these changes Oct 3, 2021

View reviewed changes

kandersolar added 2 commits October 4, 2021 16:08

preserve original test values in comment

828ddae

fix typo in comment

43aa392

kandersolar added 2 commits October 11, 2021 09:01

bump min pvlib to 0.9.0

72d64a9

add explanation to whatsnew

8905b18

campanelli-sunpower approved these changes Oct 12, 2021

View reviewed changes

campanelli-sunpower merged commit 2da9668 into SunPower:master Oct 12, 2021

kandersolar deleted the pvlib090 branch October 12, 2021 21:08

		@@ -1,4 +1,4 @@
		pvlib>=0.7.0,<0.9.0
		pvlib>=0.7.0,<0.10.0

Conversation

kandersolar commented Sep 14, 2021

Uh oh!

kandersolar commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomam commented Sep 15, 2021

Uh oh!

anomam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anomam commented Oct 3, 2021

Uh oh!

srisukhi commented Oct 3, 2021

Uh oh!

kandersolar commented Oct 4, 2021

Uh oh!

campanelli-sunpower commented Oct 6, 2021

Uh oh!

kandersolar commented Oct 6, 2021

Uh oh!

campanelli-sunpower commented Oct 7, 2021

Uh oh!

anomam commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kandersolar commented Oct 11, 2021

Uh oh!

campanelli-sunpower left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kandersolar commented Sep 14, 2021 •

edited

Loading

anomam commented Oct 10, 2021 •

edited

Loading