Skip to content

ENH: calcparams_pvsyst should output d2mutau and vbi #516

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

Open
mikofski opened this issue Aug 3, 2018 · 2 comments
Open

ENH: calcparams_pvsyst should output d2mutau and vbi #516

mikofski opened this issue Aug 3, 2018 · 2 comments

Comments

@mikofski
Copy link
Member

mikofski commented Aug 3, 2018

Problem

  • users may incorrectly use coefficients from calcparams_desoto with d2mutau in singlediode
  • is there a way to restrict or limit use of d2mutau only for coefficients derived from calcparams_pvsyst?
  • users may not correctly account for the correct number of cells in series and the the number of junctions per cell when determining builtin voltage parameter
  • the cells_in_series argument in bishop88 is redundant and inconsistent with the other argument with series cells nNsVth.

Solution
As @cwhanse suggested, replace voltage_builtin with NsVbi which is an output from calcparams_pvsyst, and d2mutau should also be passed through calcparams_pvsyst in order to discourage users from using it with CEC or DeSoto models.

We currently pass the product nNsVth into singlediode and related functions, rather than passing n, Ns, and Vth separately. With the recombination current, the only place Ns is needed separately is with the per-cell Vbi to get a module Vbi. I think a more consistent method would be to pass NsVbi as the argument, rather than Vbi. That keeps all the single diode equation parameters as 'per-module' quantities, rather than a mix of per-cell and per-module.

Alternatives
Please suggest alternatives. One idea that was proposed originally was I think to lock in specific usage with the model, ie: have a separate singlediode_desoto and singlediode_pvsyst similar to the calcparams_* methods. Not sure how far down this needs to go, would it also apply to bishop88?

Additional context
this is an follow on to #504

@adriesse
Copy link
Member

adriesse commented Aug 3, 2018

How about supplementing each set of coefficients with a "source" identifier? Then downstream functions can decide whether the "source" is suitable for them.
On the other hand this is a toolbox, and if someone want to use a hammer on a screw...

@cwhanse
Copy link
Member

cwhanse commented Aug 3, 2018

I'm thinking about writing a Module class, which would contain datasheet information, parameter sets for the various performance models (which makes a sharp separation between CEC and PVsyst parameter sets), perhaps even measured IV curves. If we write that Class, part of it would be populated by the data in the SAM files.

mikofski added a commit to mikofski/pvlib-python that referenced this issue Aug 3, 2018
* related to pvlib#516, the use of cells_in_series here is problematic
* to calculate arrays of different cells or modules with different
parameters we use np.where(is_recomb, ..., ...) 3d0deb2 but what
should cells_in_series be if d2mutau is zero? doesn't make physical
sense for it to be zero, but None doesn't work with numbers
* remove cells_in_series arg from bishop88
* remove last warning about setting cells_in_series correctly, move it
to Notes, but worded differently in the context of setting NsVbi
* replace voltage_builtin with NsVbi, default to np.inf
* fix docstring parameter descriptions
wholmgren pushed a commit that referenced this issue Aug 8, 2018
…#504)

* implementing pvsyst recombination loss current for CdTe and a:Si

* closes #163
* add constant VOLTAGE_BUILTIN = 0.9 (V) to singlediode_methods.py
* add arguments d2mutau=0, voltage_builtin=VOLTAGE_BUILTIN, and
cells_in_series to calculate i_recomb and v_recomb
* add check for d2mutau to calc i_recomb, v_recomb, and gradients
* add terms to current and gradients

* ENH: need to set grad_i_recomb if d2mutau not given

* also set v_recomb to Inf, so that 1/v_recomb is always zero
* but precalculate grad_2i_recomb only if d2mutau > 0, otherwise, use
zero

* ENH: fix need to multiply vbi by Ns, add test for recombination

* WIP: ENH: TST: BUG: in test compare desoto vs. pvsyst

* ENH: respond to comments

* change order of bishop88 args: cells_in_series to be first before
d2mutau and voltage_builtin
* hardcode PVSYST_FS495 coefficients
* don't compare pvsyst to desoto, instead compare pvsyst to fixed values
at two conditions: reference and 888[W/m^2],55[degC]
* rename test to be agnostic to module SKU
* remove testing script if __name__ == '__main__': section, not for
release

* DOC: ENH: update bishop88 documentation for pvsyst thin-film recombination loss

* DOC: ENH: update what's new

* DOC: add utf-8 file encoding

* DOC: ENH: BUG: minor doc fixes and formatting

* need to indent bullets in warning
* only need cells in series for PVSyst thin-film recombination loss
* escape math latex twice inside docstrings without raw prefix, eg:
\\tau without extra escape is a tab character, ha ha ha
* in rst, italics is a single asterisk, not underscore, thats markdown

* ENH: improve code style, fix d2mutau is array

* expand module type abbrev. in warnings
* also expand EG to For example, too terse
* cells in series can be int _or_ `None`
* since d2mutau and vbi are arrays, then ambiguous to test for zero, so
use np.where to assign conditionally instead (in two places)

* ENH: TST: use fixture instead of module-level dictionary for test

* dangerous to use module-level dictionary inside test, b/c dictionary
values could be changed inadvertently, but not a fixture
* add fixture as argument in test
* change parametrize to call fixture instead of module-level dictionary

* ENH: replace voltage_builtin with NsVbi and remove cells_in_series

* related to #516, the use of cells_in_series here is problematic
* to calculate arrays of different cells or modules with different
parameters we use np.where(is_recomb, ..., ...) 3d0deb2 but what
should cells_in_series be if d2mutau is zero? doesn't make physical
sense for it to be zero, but None doesn't work with numbers
* remove cells_in_series arg from bishop88
* remove last warning about setting cells_in_series correctly, move it
to Notes, but worded differently in the context of setting NsVbi
* replace voltage_builtin with NsVbi, default to np.inf
* fix docstring parameter descriptions
@cwhanse cwhanse added this to the 0.6.1 milestone Sep 12, 2018
@cwhanse cwhanse modified the milestones: 0.6.1, 0.7.0 Nov 5, 2018
@wholmgren wholmgren modified the milestones: 0.7.0, 0.7.1 Nov 26, 2019
@wholmgren wholmgren modified the milestones: 0.7.1, 0.7.2 Jan 10, 2020
@cwhanse cwhanse modified the milestones: 0.7.2, 0.8.0 Feb 12, 2020
@wholmgren wholmgren modified the milestones: 0.8.0, Someday Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants