Upgrade bifacial functions to work with pvfactors v1.4.1 #934

anomam · 2020-03-15T09:34:45Z

Closes compability with pvfactors 1.4 #902
I am familiar with the contributing guidelines
Tests added
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/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`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
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.

Brief description

update functions to work with pvfactors v1.4.1
remove parallel simulations as pvfactors is now fast enough

wholmgren · 2020-03-17T20:26:06Z

Should we deprecate the run_parallel_calculations, n_workers_for_parallel_calcs kwargs or just go ahead and remove them? Usually I'd vote for deprecating in 0.7.2 and removing in 0.8, but in this case it might be better to just go ahead and remove them.

pvlib/bifacial.py

anomam · 2020-03-30T14:20:59Z

Thanks @anomam!

Should we deprecate the run_parallel_calculations, n_workers_for_parallel_calcs kwargs or just go ahead and remove them? Usually I'd vote for deprecating in 0.7.2 and removing in 0.8, but in this case it might be better to just go ahead and remove them.

Hi @wholmgren , thanks for the message and sorry for the delay.
I felt very inclined to remove it because I find the reporting classes very messy in parallel mode, and also because there's really not much of a reason to keep the parallel mode around in pvlib when the normal mode runs in a few seconds (parallel mode is not faster in most cases).
I keep the parallel mode in pvfactors mostly bc of ideas of future development I have for the package, but nothing that will happen in the foreseeable future.

On the other hand I do want to follow pvlib's best practices and I can bring it back and add a deprecation message to it if that's what the team needs. Please let me know!

wholmgren · 2020-03-30T15:55:00Z

Sorry I wasn't clear. I'm all for removing the parallel mode, but wondering if we should keep those kwargs in pvlib until 0.8. The kwargs would become non-functional and only serve to emit a warning. The purpose would be to avoid breaking user code. But I don't have a strong feeling about whether or not it is worth the effort in this case.

anomam · 2020-04-05T07:58:06Z

Sorry I wasn't clear. I'm all for removing the parallel mode, but wondering if we should keep those kwargs in pvlib until 0.8. The kwargs would become non-functional and only serve to emit a warning. The purpose would be to avoid breaking user code. But I don't have a strong feeling about whether or not it is worth the effort in this case.

@wholmgren , thanks a lot for clarifying. I added a couple deprecation warnings in 7ba10f8. I hope this works!

@cwhanse , I think that this PR is ready for another review. Please let me know if you need any additional changes.

wholmgren · 2020-04-16T15:21:32Z

pvlib/bifacial.py

-    front_poa_irradiance: numeric
+    poa_front: numeric
        Calculated incident irradiance on the front surface of the PV modules
        (W/m2)
-    back_poa_irradiance: numeric
+    poa_back: numeric
        Calculated incident irradiance on the back surface of the PV modules
        (W/m2)
-    df_registries: pandas DataFrame
-        DataFrame containing detailed outputs of the simulation; for
-        instance the shapely geometries, the irradiance components incident on
-        all surfaces of the PV array (for all timestamps), etc.
-        In the pvfactors documentation, this is refered to as the "surface
-        registry".
+    poa_front_absorbed: numeric
+        Calculated absorbed irradiance on the front surface of the PV modules
+        (W/m2), after AOI losses
+    poa_back_absorbed: numeric
+        Calculated absorbed irradiance on the back surface of the PV modules
+        (W/m2), after AOI losses


If I understand the diff correctly, this change to the output will break user code. Ok with me but we should note it in the API Changes section. I guess there's not much point in deprecating the parallel kwargs if we're just changing the return type, so maybe best to just make users deal with the pain in the next release. I'm guessing that users of the bifacial module are relatively tolerant of api changes.

Thanks for the feedback @wholmgren :

I could revert the output change and return some kind of placeholder for df_registries (pvfactors v1.4.1 does not use registries anymore). Or I can just update the API changes section. Just let me know what you prefer!

I don't mind removing the deprecation warnings as well

In this case I think we're better off immediately conforming to the pvfactors 1.4 API, provided the API change is documented. Any objections?

Hi @wholmgren , sorry for the delay. I removed the deprecated inputs and warnings, and updated the API changes section of the newest whatsnew file. Please let me know if you need any other fixes

wholmgren

Thanks @anomam!

Tests are now passing with pvfactors v1.4.1

7328c26

cwhanse reviewed Mar 17, 2020

View reviewed changes

pvlib/bifacial.py Outdated Show resolved Hide resolved

CameronTStark mentioned this pull request Mar 25, 2020

v0.7.2 release planning #864

Closed

anomam added 2 commits March 30, 2020 23:03

Update docstrings names for returned variables per pvlib convention

f740dea

Bump pvfactors version in ci config for testing

b4d74c8

anomam added 3 commits April 5, 2020 16:42

Add deprecation warning for parallel mode inputs

7ba10f8

Update whatsnew file and remove unused import

40e9ccc

Typo

c883437

CameronTStark added this to the 0.7.2 milestone Apr 5, 2020

CameronTStark added the bug label Apr 5, 2020

cwhanse approved these changes Apr 16, 2020

View reviewed changes

wholmgren reviewed Apr 16, 2020

View reviewed changes

CameronTStark modified the milestones: 0.7.2, 0.7.3 Apr 23, 2020

CameronTStark removed this from the 0.7.3 milestone May 26, 2020

anomam added 4 commits June 18, 2020 19:51

Merge branch 'master' into upgrade_pvfactors

014cda2

Remove deprecation warnings and unused inputs

607bdef

Updated whatsnew file

3f938ed

Remove changes from old whatsnew file

ffb8209

anomam requested a review from wholmgren June 18, 2020 11:35

wholmgren approved these changes Jun 18, 2020

View reviewed changes

wholmgren merged commit 05b3209 into pvlib:master Jun 18, 2020

kandersolar mentioned this pull request Jul 7, 2020

Why remove parallelization for bifacial.py? #997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade bifacial functions to work with pvfactors v1.4.1 #934

Upgrade bifacial functions to work with pvfactors v1.4.1 #934

anomam commented Mar 15, 2020 •

edited by CameronTStark

Loading

wholmgren commented Mar 17, 2020

anomam commented Mar 30, 2020

wholmgren commented Mar 30, 2020

anomam commented Apr 5, 2020

wholmgren Apr 16, 2020

anomam Apr 17, 2020

wholmgren Apr 17, 2020

anomam Jun 18, 2020

wholmgren left a comment

Upgrade bifacial functions to work with pvfactors v1.4.1 #934

Upgrade bifacial functions to work with pvfactors v1.4.1 #934

Conversation

anomam commented Mar 15, 2020 • edited by CameronTStark Loading

wholmgren commented Mar 17, 2020

anomam commented Mar 30, 2020

wholmgren commented Mar 30, 2020

anomam commented Apr 5, 2020

wholmgren Apr 16, 2020

Choose a reason for hiding this comment

anomam Apr 17, 2020

Choose a reason for hiding this comment

wholmgren Apr 17, 2020

Choose a reason for hiding this comment

anomam Jun 18, 2020

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

anomam commented Mar 15, 2020 •

edited by CameronTStark

Loading