MAINT: DOC: rename singlediode module, and clean up docstring for singlediode function #525

mikofski · 2018-08-08T23:32:44Z

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 ENH: rename singlediode_methods.py to singlediode.py and discuss pvsystem.singlediode refactor #515 and closes DOC: remove very long voc estimate equations from singlediode #518
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):

rename singlediode_methods.py to just singlediode.py
change usage in pvsystem to full namespace to avoid clash with existing method
move long description of Bishop algorithm and equations to new singlediode rst-file and add to index

* closes pvlib#515 * import pvlib in pvsystem and use pvlib.singlediode to differentiate from existing pvsystem.singldiode() method already there! * rename test_singlediode_methods.py to test_singlediode.py * update documentation and comments

* closes pvlib#518 * add singlediode.rst and add to index

* proofed the docs and they are ok

mikofski · 2018-08-08T23:50:08Z

okay, @wholmgren and @cwhanse can you take a look? thx

wholmgren

I like it. Minor comments below.

wholmgren · 2018-08-08T23:55:59Z

pvlib/pvsystem.py

@@ -20,7 +20,7 @@
 from pvlib.tools import _build_kwargs
 from pvlib.location import Location
 from pvlib import irradiance, atmosphere
-from pvlib import singlediode_methods
+import pvlib  # FIXME: import singlediode module from pvlib


why FIXME? seems ok to me.

Something like from pvlib import singlediode as _singlediode would work too.

✅ removed fixme, replace with comment use pvlib.singlediode to avoid clash with local method

wholmgren · 2018-08-08T23:58:20Z

docs/sphinx/source/singlediode.rst

@@ -0,0 +1,104 @@
+.. _singlediode:


Thanks for adding this! Perhaps it deserves its own note in the whatsnew document.

Didn't you make something else (a wiki?) with more information that might belong here?

Are you set up to get readthedocs to render this version? It's pretty easy to do if you're not. Always nice to see that it works when making bigger documentation changes. If you've rendered locally and say it looks good then ok with me.

Yes, there is a wiki on cell model solutions, but in order to close #518, I just need to move the section on bishop88() out of singlediode(), so I would like to open a separate documentation issue/pr to address moving the material appropriate from the wiki to the documents, and perhaps augmenting that with more detail as needed. Is this okay?

✅ added note under documentation in what's new:

Added section on single diode equation with some detail on solutions used in
pvlib-python (:issue:518)

yes, that's ok with me.

wholmgren · 2018-08-09T00:01:03Z

docs/sphinx/source/singlediode.rst

+pvlib-python to generate an IV curve of a PV module.
+
+pvlib-python supports two ways to solve the single diode model, by passing the
+a ``method`` keyword to the :func:`pvlib.pvsystem.singlediode` function:


I suggest something like...

pvlib-python supports two ways to solve the single diode model:

The :func:pvlib.pvsystem.singlediode function allows the user to choose the method using the method keyword.

Agree with @wholmgren here

✅ nice change thx!

cwhanse · 2018-08-09T14:57:22Z

docs/sphinx/source/singlediode.rst

+Single Diode Model
+==================
+
+This section reviews the solutions to the single diode model used in


solutions to the single diode equation here and elsewhere. Let's use "Single diode equation" when we are referring to the equation that describes a single IV curve, and "single diode model" when we are referring to the set of equations (one of which is the single diode equation) that describe IV curves at any irradiance and temperature condtion (e.g., PVsyst is a single diode model).

These are my preferred terms, and the meaning of 'model' can be debated at length without adding value. I'm pushing here for consistency and hopefully, clarity.

✅ agreed, I changed all references for now to single diode equations, since I don't discuss any single diode models here yet eg: PVSyst or DeSoto

cwhanse · 2018-08-09T15:10:13Z

docs/sphinx/source/singlediode.rst

+:math:`f \left( w \right) = w \exp \left( w \right)` or
+:math:`w = f^{-1} \left( w \exp \left( w \right) \right)` also given as
+:math:`w = W \left( w \exp \left( w \right) \right)`. Rearranging the single
+diode equation above with a Lambert W-function yields the following.


I'd replace the phrase "Rearranging the single diode equation above with a Lambert W-function yields the following. " with "Defining"

Because I could not find a derivation of the Lambert W solution in literature, I wrote it out in a Sandia report. We could add a reference if you want.

Is it this reference? "Parameter Estimation for Single Diode Models of Photovoltaic Modules," Clifford W. Hansen, Sandia Report SAND2015-2065, 2015

✅ OK, I think I've reworded it using "Defining ..." is this better:

Defining the following parameter, :math:z, is necessary to transform the single diode equation into a form that can be expressed as a Lambert W-function.

cwhanse · 2018-08-09T15:10:50Z

docs/sphinx/source/singlediode.rst

+       \frac{R_s \left( I_L + I_0 \right) + V}{n Ns V_{th} \left(1 + \frac{R_s}{R_{sh}}\right)}
+       \right)
+
+The the module current can be solved using the Lambert W-function.


cwhanse · 2018-08-09T15:12:47Z

docs/sphinx/source/singlediode.rst

+current is also negative, meaning that the corresponding voltage must be
+in the 4th quadrant and therefore greater than the open circuit voltage
+(see proof below). Therefore the entire forward-bias 1st quadrant IV-curve
+is bounded, and a bisection search within these points will always find


please clarify "these points" I believe you mean "a bisection search for V_oc between V_d = 0 and V_{oc, est}"

✅ I tried to make this wording better. I added between Vd and Voc,est, but the point is not just to find Voc, but to be able to find any 1st quadrant point including Voc.

cwhanse · 2018-08-09T15:15:31Z

docs/sphinx/source/singlediode.rst

+pvlib-python to generate an IV curve of a PV module.
+
+pvlib-python supports two ways to solve the single diode model, by passing the
+a ``method`` keyword to the :func:`pvlib.pvsystem.singlediode` function:


Agree with @wholmgren here

* add to what's new documentation section that there is now some detail on pvlib-python solutions to single diode equation * change single diode _model_ to single diode _equation_ everywhere to distinguish between the equation I = IL - I0*(exp(Vd/nNsVt)-1) - Vd/Rsh and implementation like PVSyst and DeSoto that derive coefficients * add reference to Cliff's Sandia Report on Lambert W-function solution of single diode equation * remove fixme in pvsystem

mikofski · 2018-08-10T00:35:02Z

OK @wholmgren and @cwhanse can you take another look? thanks!

mikofski · 2018-08-10T00:37:38Z

not a failure, conda servers timedout

wholmgren · 2018-08-10T01:29:03Z

Looks good to me. Maybe I missed it, but did you confirm that the documentation renders correctly?

mikofski · 2018-08-10T01:34:02Z

Sorry, yes I confirmed the docs render correctly

cwhanse

All edits are fine. Yes that’s the reference I had in mind.

wholmgren · 2018-08-10T03:19:28Z

Thanks Mark!

mikofski added 3 commits August 8, 2018 14:16

DOC: move proof that estimated Voc is in q4

1a42153

* closes pvlib#518 * add singlediode.rst and add to index

DOC: add DOI to Jain and Kapoor

a49e959

* proofed the docs and they are ok

wholmgren reviewed Aug 9, 2018

View reviewed changes

cwhanse reviewed Aug 9, 2018

View reviewed changes

mikofski added 2 commits August 9, 2018 17:16

DOC: MAINT: fix citation for Sandia Report, remove extra parentheses

fa03a93

cwhanse approved these changes Aug 10, 2018

View reviewed changes

wholmgren merged commit dc6bdbf into pvlib:master Aug 10, 2018

mikofski deleted the singlediode_gh515_gh518 branch August 10, 2018 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: DOC: rename singlediode module, and clean up docstring for singlediode function #525

MAINT: DOC: rename singlediode module, and clean up docstring for singlediode function #525

mikofski commented Aug 8, 2018 •

edited

Loading

mikofski commented Aug 8, 2018

wholmgren left a comment

wholmgren Aug 8, 2018

mikofski Aug 9, 2018

wholmgren Aug 8, 2018

wholmgren Aug 9, 2018

mikofski Aug 9, 2018 •

edited

Loading

mikofski Aug 9, 2018

wholmgren Aug 9, 2018

wholmgren Aug 9, 2018

cwhanse Aug 9, 2018

mikofski Aug 9, 2018

cwhanse Aug 9, 2018

mikofski Aug 9, 2018

cwhanse Aug 9, 2018

mikofski Aug 9, 2018

mikofski Aug 9, 2018

cwhanse Aug 9, 2018

mikofski Aug 9, 2018

cwhanse Aug 9, 2018

mikofski Aug 10, 2018

cwhanse Aug 9, 2018

mikofski commented Aug 10, 2018

mikofski commented Aug 10, 2018

wholmgren commented Aug 10, 2018

mikofski commented Aug 10, 2018

cwhanse left a comment

wholmgren commented Aug 10, 2018

MAINT: DOC: rename singlediode module, and clean up docstring for singlediode function #525

MAINT: DOC: rename singlediode module, and clean up docstring for singlediode function #525

Conversation

mikofski commented Aug 8, 2018 • edited Loading

pvlib python pull request guidelines

mikofski commented Aug 8, 2018

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski commented Aug 10, 2018

mikofski commented Aug 10, 2018

wholmgren commented Aug 10, 2018

mikofski commented Aug 10, 2018

cwhanse left a comment

Choose a reason for hiding this comment

wholmgren commented Aug 10, 2018

mikofski commented Aug 8, 2018 •

edited

Loading

mikofski Aug 9, 2018 •

edited

Loading