Skip to content
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

ENH: rename singlediode_methods.py to singlediode.py and discuss pvsystem.singlediode refactor #515

Closed
mikofski opened this issue Aug 3, 2018 · 4 comments · Fixed by #525
Labels
Milestone

Comments

@mikofski
Copy link
Member

mikofski commented Aug 3, 2018

Problem
Rename module name singlediode_methods.py and refactor pvsystem.singlediode

  • singlediode_methods.py is a long name for a module
  • the suffix "methods" is ambiguous, superfluous, and not descriptive
  • the other modules have meaningful names that correspond loosely to the modeling steps described at Sandia PVPMC
  • there needs to be a discussion regarding possibly refactoring pvsystem.singlediode
    • having a module shadow a method from a different module might be confusing or maybe it's not a problem?
    • has there already been discussion on breaking singlediode up, refactoring, or renaming it?

Proposed Solution
Rename module singlediode_methods.py to just singlediode.py and consider refactoring pvsystem.singlediode

  1. rename the module from singlediode_methods.py to just singlediode.py
  2. change all references in the code where appropriate
  3. change all references in the comments and documentation where appropriate
  4. start a discussion on the repercussions to the singlediode() methods in pvsystem.py

alternatives

  • rename the module something else like dc_energy.py, pvmodules.py, or pvcells.py
  • do nothing
  • rename the module to singlediode.py and leave pvsystem.singlediode() alone
  • rename the module to singlediode.py and move pvsystem.singlediode() to singlediode.singlediode() which is a more established pattern, right?

Additional context

@wholmgren wholmgren added this to the 0.6.0 milestone Aug 3, 2018
@wholmgren wholmgren added the api label Aug 3, 2018
@cwhanse
Copy link
Member

cwhanse commented Aug 3, 2018

My mental model has the following use cases:

  • calculate IV curve parameters for a specific single diode model at given irradiance and temperature conditions. This is the job of calcparams_XXXX, there's no way to avoid multiple functions because the single diode models use different sets of equations.
  • calculate an IV curve for a given set of IV curve parameters. This is the job of singlediode which should accept a kwarg to select a calculation method, e.g., lambertw, bishop88, others perhaps @thunderfish24. The standard single diode equation (SDE) and SDE+recombination are different. If possible, we should handle calculations for the SDE and SDE+recombination within a IV curve method: bishop88 can and is the subject of ENH: implementing pvsyst recombination loss current for CdTe and a:Si #504. Extending other methods to accommodate the recombination term is requested by this issue. However, I already know that the solution technique used in the lambertw method cannot be extended to SDE+recombination.
  • ModelChain should have a method that conveniently wraps the various DC models, including sapm, Desoto, CEC, PVsyst. We have a partial implementation in ModelChain.dc_model.

Regarding #517, I vote strongly against singlediode_pvsyst unless we can't find any other workable way to host the several single diode models.

@wholmgren
Copy link
Member

I suggest we rename singlediode_methods.py to singlediode.py as soon as possible. I think this is a blocker for 0.6 but the rest of it could be pushed off to 0.7.

@cwhanse
Copy link
Member

cwhanse commented Aug 8, 2018

Ok.

@mikofski
Copy link
Member Author

mikofski commented Aug 8, 2018

FYI: I'm on it right now, and BTW: Cliff was right, they clashed in pvsystem.py, but it's no problem.

mikofski added a commit to mikofski/pvlib-python that referenced this issue Aug 8, 2018
* 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
wholmgren pushed a commit that referenced this issue Aug 10, 2018
…glediode function (#525)

* MAINT: rename singlediode_methods.py to just singlediode.py

* closes #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

* DOC: move proof that estimated Voc is in q4

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

* DOC: add DOI to Jain and Kapoor

* proofed the docs and they are ok

* DOC: MAINT: improve wording in singlediode.rst

* 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

* DOC: MAINT: fix citation for Sandia Report, remove extra parentheses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants