Add desoto and pvsyst as dc_model options to ModelChain by cwhanse · Pull Request #555 · pvlib/pvlib-python

cwhanse · 2018-09-04T16:55:22Z

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

Closes issue Add PVsyst as option to ModelChain #487
I am familiar with the contributing guidelines.
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

wholmgren · 2018-09-04T18:12:08Z

+                                  'ModelChain object. singlediode is '
+                                  'ambiguous, use desoto instead. singlediode '
+                                  'keyword will be removed in v0.7.0 and '
+                                  'later', DeprecationWarning)


This should be a pvlibDeprecationWarning.

Can you also look into adding a test that will fail on pvlib versions >= 0.7? See test_irradiance.py::test_deprecated_07 for an example.

Near the top of both modelchain.py and test_modelchain.py:

from pvlib._deprecation import pvlibDeprecationWarning

On line 22 of test_modelchain.py:

from conftest import fail_on_pvlib_version, requires_scipy

wholmgren · 2018-09-04T18:15:33Z

+
+        self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0)
+
+        return self


I don't love the very similar copy/paste code. But it's explicit and that is important too. So I lean towards this approach.

I would mark this with FIXME. IMO the copy/paste only seems explicit, but it is WET code. DRY code is robust and not prone to errors. I agree this is probably maintainable in the short term, but I think it might become a burden in the future, especially if more "models" are added, such as pvsyst_v7, ...

There are only two lines of difference between these:

--- desoto.py +++ pvsyst.py @@ -1,7 +1,7 @@ -def desoto(self): +def pvsyst(self): (photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth) = ( - self.system.calcparams_desoto(self.effective_irradiance, + self.system.calcparams_pvsyst(self.effective_irradiance, self.temps['temp_cell'])) self.diode_params = (photocurrent, saturation_current,

You could add a tiny dispatcher that you delegate this common task to that is DRY and IMHO also explicit:

def desoto(self): """calculate desoto model""" return self._model_dispatcher(self.system.calcparams_desoto, 'desoto') def pvsyst(self): """calculate pvsyst model""" return self._model_dispatcher(self.system.calcparams_pvsyst, 'pvsyst') def _model_dispatcher(self, model_calcparams, model_name=None): """ Delegate that dispatches the model Parameters ---------- model_calcparams : function A function that calculates the models parameters given irradiance and cell temperature. model_name : str Optional string that may be used to further distinguish the model in the future. """ (photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth) = ( model_calcparams(self.effective_irradiance, self.temps['temp_cell'])) self.diode_params = (photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth) self.dc = self.system.singlediode( photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth) self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0) return self

Either approach is ok with me at this point. Keep in mind that we've had discussions elsewhere about whether or not it even makes sense to define these model-specific methods on ModelChain and if we could replace them with wrappers on PVSystem. So I'm not too concerned about perfection here.

okay, me too, I fine either way 😁

@mikofski that's a good suggestion. You are right that we have more diode models waiting - CEC hasn't been connected up yet. But we will (at least I hope) also have system.doublediode someday, and perhaps other variations on the calculate parameters -> calculate IV curve pattern.

Let's keep the bite small, merge this, and take up improvements in another pull request.

wholmgren · 2018-09-05T15:48:01Z

Nice job modifying the test.

I think we should remove the deprecated method from api.rst instead of adding (deprecated) after the method reference. The sphinx build process does not render that in the way you're expecting it to...

$ git remote add cwhanse git@github.com:cwhanse/pvlib-python.git
$ git fetch cwhanse
remote: Counting objects: 182, done.
remote: Compressing objects: 100% (32/32), done.
remote: Total 182 (delta 122), reused 128 (delta 109), pack-reused 41
Receiving objects: 100% (182/182), 88.10 KiB | 1.69 MiB/s, done.
Resolving deltas: 100% (124/124), completed with 34 local objects.
From github.com:cwhanse/pvlib-python
 * [new branch]      CEC               -> cwhanse/CEC
 * [new branch]      Egref             -> cwhanse/Egref
 * [new branch]      firstsolar        -> cwhanse/firstsolar
 * [new branch]      forecast          -> cwhanse/forecast
 * [new branch]      forecast-tools    -> cwhanse/forecast-tools
 * [new branch]      io                -> cwhanse/io
 * [new branch]      master            -> cwhanse/master
 * [new branch]      modelchain        -> cwhanse/modelchain
 * [new branch]      patch-1           -> cwhanse/patch-1
 * [new branch]      samupdate         -> cwhanse/samupdate
 * [new branch]      singlediode-chain -> cwhanse/singlediode-chain
$ git checkout CEC
Branch 'CEC' set up to track remote branch 'CEC' from 'cwhanse'.
Switched to a new branch 'CEC'
$ conda activate pvlibdocs
(pvlibdocs) $ cd docs/sphinx
(pvlibdocs) $ make clean; make html
(pvlibdocs) $ open build/html/index.html

Alternatively, you could add the docstring """Deprecated""" to the method definition in modelchain.py and it would look like this:

wholmgren · 2018-09-05T15:53:19Z

+                                          self.temps['temp_cell']))
+
+        self.diode_params = (photocurrent, saturation_current, resistance_series,
+                       resistance_shunt, nNsVth)


Should read as:

self.diode_params = (photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth)

same in pvsyst below. Not sure why the git diff/flake8 command doesn't pick up on this. Maybe time for another github/CI integration tool.

Fixed here and in pvsyst

This reverts commit 7bf8a30.

wholmgren · 2018-09-05T16:23:29Z

Great. I think this is ready to merge.

cwhanse added 2 commits September 4, 2018 10:52

Add desoto and pvsyst as dc_model options to ModelChain

b4737d3

Add desoto to DC_MODEL_PARAMS

e7fb500

wholmgren reviewed Sep 4, 2018

View reviewed changes

Change to pvlibDeprecationWarning, add v0.7 test for singlediode keyword

4c621c6

wholmgren added this to the 0.6.0 milestone Sep 4, 2018

wholmgren added the enhancement label Sep 4, 2018

cwhanse added 6 commits September 4, 2018 13:47

Fix model function reference in test_infer_dc_model

9777ba6

Change function object to strings

f2d8fe1

Add pvsyst_dc_snl_ac_system to test_infer_dc_models argument list

3a3afc6

Add sam_data argument to pvsyst_dc_snl_ac_system

df3a55f

Update whatsnew, api.rst

219e8f4

Merge branch 'master' of https://github.com/pvlib/pvlib-python into CEC

db66d66

wholmgren reviewed Sep 5, 2018

View reviewed changes

cwhanse added 4 commits September 5, 2018 09:53

Add deprecated to ModelChain.singlediode docstring

7bf8a30

Revert "Add deprecated to ModelChain.singlediode docstring"

b5ed45f

This reverts commit 7bf8a30.

Add deprecated to ModelChain.singlediode docstring

85e9aac

Fix indents, remove ModelChain.singlediode from api.rst

230349c

cwhanse merged commit cba621b into pvlib:master Sep 5, 2018

wholmgren mentioned this pull request Sep 8, 2018

convert modelchain.ipynb to rst, remove nbsphinx #568

Merged

8 tasks

cwhanse mentioned this pull request Sep 12, 2018

Add PVsyst as option to ModelChain #487

Closed

cwhanse deleted the CEC branch September 21, 2018 20:30


		self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0)

		return self

Uh oh!

Conversation

cwhanse commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pvlib python pull request guidelines

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikofski Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

wholmgren commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wholmgren commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cwhanse commented Sep 4, 2018 •

edited

Loading

mikofski Sep 5, 2018 •

edited

Loading