-
Notifications
You must be signed in to change notification settings - Fork 1.1k
ENH: implementing pvsyst recombination loss current for CdTe and a:Si #504
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: implementing pvsyst recombination loss current for CdTe and a:Si #504
Conversation
* closes pvlib#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
* 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
@cwhanse the difference between DeSoto/CEC versus PVSyst with recombination losses seem correct now. See below for plots and and the next comment for the PVSyst First Solar FS-495 parameters used. |
@cwhanse if you think the ranges for these differences seems okay, then it is working for So, do you and @wholmgren think we should now add the Also, @cwhanse sorry to raise a dead issue, but Will and I both agree that |
Can you post the other PVsyst parameters for this module, and I'll check against a MATLAB implementation? My quibble with changing |
For DeSoto I got the parameters from In [8]: fs_495
Out[8]:
BIPV N
Date 9/18/2014
T_NOCT 44.6
A_c 0.72
N_s 216
I_sc_ref 1.55
V_oc_ref 86.5
I_mp_ref 1.4
V_mp_ref 67.9
alpha_sc 0.000924
beta_oc -0.22741
a_ref 2.9482
I_L_ref 1.563
I_o_ref 2.64e-13
R_s 6.804
R_sh_ref 806.27
Adjust -10.65
gamma_r -0.264
Version NRELv1
PTC 89.7
Technology CdTe
Name: First_Solar_FS_495, dtype: object For PVSyst First Solar FS-495 I used this: PVSYST_FS_495
Out[14]:
{'d2mutau': 1.31,
'alpha_sc': 0.00039,
'gamma_ref': 1.48,
'mu_gamma': 0.001,
'I_o_ref': 9.62e-10,
'R_sh_ref': 5000,
'R_sh_0': 12500,
'R_sh_exp': 3.1,
'R_s': 4.6,
'beta_oc': -0.2116,
'EgRef': 1.475,
'cells_in_series': 108,
'cells_in_parallel': 2,
'I_sc_ref': 1.55,
'V_oc_ref': 86.5,
'I_mp_ref': 1.4,
'V_mp_ref': 67.85,
'temp_ref': 25,
'irrad_ref': 1000,
'I_L_ref': 1.5743233463848496} Ok, I'll await further changes to the thin film recombination loss current until I get a thumbs up. RE: the other matter, I guess let's leave it as |
@wholmgren , @cwhanse after talking with @jdnewmil I realized I should not have used parameters from CEC database - and also I used the wrong method, I should have used the new I've revised the test and plots now, and posted them above. Thanks! |
@mikofski I'm having trouble interpreting your figures above. Does DeltaI compare the Desoto model to the Pvsyst model? I would like to verify that the results with the recombination term appear reasonable, and to do that it would help to compare two IV curves computed with the same model (Pvsyst) without and with the term activated. |
OK, but AFAIK you can't use PVSyst this way, b/c the parameters would be different ie using the PVSyst database parameters for FS-495 without the recombination loss will not fit the measured IV curve or nameplate output power. What my plots above show is that the PVSyst model agrees with the DeSoto model at the critical points (short circuit, max power, open circuit) when the correct parameters for each are used respectively, but that the shape of the IV curve is different, especially between Vmp and Voc which is where Merten observed differences between model and measurements. Here are the deltas for PVSyst model with and without recombination loss current:
and plots comparing PVSyst model with and without recombination loss current: NOTE: The same PVSyst model parameters were used for both conditions, however, PVSyst model is not intended to be used without the recombination loss current for CdTe or amorphous-silicon modules, therefore the calculate IV curve and max power without recombination loss are wrong. |
thanks @mikofski I'm more interested to qualitatively verify the with/without recombination term calculation than to compare Desoto and Pvsyst for nominally the same module. The results in the Pvsyst with/without figures look correct. Comparing Desoto to Pvsyst brings in other assumptions in addition to the recombination current, not least of which is the method used to derive the model parameters. I'm ready to merge this, pending concurrence from @wholmgren |
@mikofski let me know when the PR template boxes are checked and I will review. |
Hi @wholmgren, I don't think this is ready yet. I still need some direction on how many other methods these parameters should be added to, and what type of text we want to put in the documentation for users. I think we would need to add
I think in addition to docstring API changes that describe the new arguments, we need some sternly worded messages that warn users:
... and ...
... and ...
EG: FS-495 has two parallel sub-strings with 108 cells each, therefore Can someone comment to let me know if these changes are aligned with their thoughts? thx! |
@mikofski ah, I haven't followed closely and was only responding to @cwhanse's comment regarding ready to merge. Maybe there is a mismatch in anticipated scope? Re |
Ok, so then what should we do with this PR and do I need to open a few new issues to start discussion of these remaining tasks? |
pvlib/singlediode_methods.py
Outdated
@@ -62,7 +64,9 @@ def estimate_voc(photocurrent, saturation_current, nNsVth): | |||
|
|||
|
|||
def bishop88(diode_voltage, photocurrent, saturation_current, | |||
resistance_series, resistance_shunt, nNsVth, gradients=False): | |||
resistance_series, resistance_shunt, nNsVth, d2mutau=0, | |||
cells_in_series=None, voltage_builtin=VOLTAGE_BUILTIN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit - re-order the arguments to keep the non-recombination current terms first, followed by d2mutau
and voltage_builtin
- just move cells_in_series
up one position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
|
||
def pvsyst_fs_495(): | ||
""" | ||
PVSyst First Solar FS-495 parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculate PVsyst parameters for First Solar FS-495 module.
Actually I would prefer to simply hard-code the dict of results, and provide a brief statement for the source of these values - where does the initial fs_495
data come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ the initial data comes from the PVSyst v6.7.2 database, I don't think any of the values were altered but it was vetted internally.
I've removed the method, and hard coded the values with a comment re: initial source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having a current PVsyst license, I'm guessing that it does NOT permit uploading that database to pvlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not the whole database, but I'm guessing the relevant values come from pan files generated by FS.
PVSYST_FS_495 = pvsyst_fs_495() # PVSyst First Solar FS-495 parameters | ||
|
||
|
||
def test_pvsyst_fs_495_recombination_loss(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test is more general than just the fs_495
module - I'd drop that nomenclature from the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ agreed, changed to test_pvsyst_recombination_loss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can rename the module to singlediode
. We might want to refactor the function singlediode
but I am in favor of keeping it as a wrapper that accepts the coefficients for the single diode equation and return the IV curve. That functionality is generic to many of the DC models we implement. We could rename the function, I suppose.
I'm in favor of merging this pull request with its current scope (after approvals). We can open separate issues to 1) add voltage_builtin
('Vbi') and d2mutau
to the component functions, 2) clarify the cells_in_series
(aka Ns
) argument. 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.
I'm not sure we need those 'sternly worded' warnings. We could test inside calcparams_pvsyst
for cell type and non-zero d2mutau
and warn the user when the recombination current term isn't set up right. But to me, the real issue is abeting the view that the parameters in the CEC model are somehow generic for any single diode model. I think a way to ameliorate that risk is tie the SAM parameter reader function to the CEC model, so that it is awkward to invoke the PVsyst model (or other single diode model) on a set of parameters read from the SAM file.
) | ||
|
||
# test expected change in max power | ||
assert np.isclose(max(desoto[2]) - max(pvsyst[2]), 0.01949420697212645) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes if the Desoto model and the PVsyst model are 'close', meaning that the test would fail is the CEC parameter database entry for this module changes, if there is an error in the Desoto functions, or if the PVsyst function has an error. I'd change the test condition to compare the PVsyst output to known values calculated by a separate method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ I've changed the test to be independent of DeSoto, now it only tests 2 conditions:
- reference condition, should agree with its own parameters
- another condition, (888[W/m^2], 55[degC]), for which I calculated the expected output using
calcparams_pvsyst
andbishop88
But I should note that the output at (888[W/m^2], 55[degC]) doesn't agree well with DeSoto at the same conditions:
In [1]: from pvlib import pvsystem
In [2]: CECMOD = pvsystem.retrieve_sam('cecmod')
In [3]: fs_495 = CECMOD.First_Solar_FS_495
In [4]: il, io, rs, rsh, nnsvt= pvsystem.calcparams_desoto(888, 55,
...: alpha_sc=fs_495.alpha_sc, a_ref=fs_495.a_ref, I_L_ref=fs_495.I_L_ref,
...: I_o_ref=fs_495.I_o_ref, R_sh_ref=fs_495.R_sh_ref, R_s=fs_495.R_s,
...: EgRef=1.475, dEgdT=-0.0003)
In [5]: pvs = pvsystem.singlediode(il, io, rs, rsh, nnsvt, method='lambertw')
In [6]: pvs
Out[6]:
OrderedDict([('i_sc', 1.4020527838304147), # PVSyst: 1.3868344548308347
('v_oc', 75.41704855182024), # PVSyst: 79.29198489135703
('i_mp', 1.2599626119945482),
('v_mp', 57.70813302772382),
('p_mp', 72.71009002293975), # PVSyst: 76.26211540956041
('i_x', 1.3606253515830833),
('i_xx', 0.8340096657730403)])
According to @jdnewmil this is possibly due to a setting in PVSyst that adjusts d2mutau
to a fixed max power temp coeff instead (or in addition???) to the adjustment on gamma
. Even though I can see these check boxes in PVSyst, I'm not sure to what degree we want to copy or guess what PVSyst does here in PVLib (or in SAM either @timorichert)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwhanse if we wanted to try to emulate this 2nd order d2mutau
behavior, then IMO it should go into calcparams_pvsyst
, and so those values: d2mutau
(and voltage_builtin
if multi junction?) should be outputs from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I don't know what PVsyst might be doing behind the interface when d2mutau
is in action, however, so I can't help clarify here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't clear what PVsyst is doing, then I suggest mailing Bruno to get clarification. I think they're quite open about the calculations.
return desoto, pvsyst | ||
|
||
|
||
if __name__ == '__main__': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although great for development and testing, I believe we don't keep if __name__ == '__main__':
blocks in modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ agreed, removed
* 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
okay, @wholmgren @cwhanse and anyone else, I think this is ready for review and merging if there are no more comments. thanks |
Evidently Python-2 doesn't like "André Mermoud" because it had utf-8, let me add encoding to the file and see if that fixes it |
emphasis mine:
I think a possible solution to this situation is for NOTE: having And this could also avoid any confusion regarding CIS/CIGS which should not use the thin-film recombination loss at all.
I agree with everything in the second paragraph.
Thank you. So shall this also be a separate issue out of scope for this PR? |
* 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
failures are do to Siphon, not related to doc changes in 1e9434a |
We agree.
Yes, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikofski this looks good. Only minor changes requested. I appreciate the limited scope!
pvlib/singlediode_methods.py
Outdated
@@ -86,6 +95,18 @@ def bishop88(diode_voltage, photocurrent, saturation_current, | |||
nNsVth : numeric | |||
product of thermal voltage ``Vth`` [V], diode ideality factor ``n``, | |||
and number of series cells ``Ns`` | |||
cells_in_series : int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None or int
pvlib/singlediode_methods.py
Outdated
.. warning:: | ||
* Do not use ``d2mutau`` with CEC coefficients. | ||
* Usage of ``d2mutau`` with PVSyst coefficients is required for CdTe and | ||
a:Si modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about the short hand for module type here. I suppose this is fairly low level function so we should not worry too much.
pvlib/singlediode_methods.py
Outdated
The PVSyst thin-film recombination losses parameters ``d2mutau`` and | ||
``voltage_builtin`` are only applied to cadmium-telluride (CdTe) and | ||
amorphous-silicon (a:Si) PV modules, [2]_, [3]_. The builtin voltage should | ||
account for all junctions. *EG*: tandem and triple junction cell would have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EG -> For example
pvlib/singlediode_methods.py
Outdated
i_recomb, v_recomb = 0, np.inf | ||
if d2mutau > 0: | ||
v_recomb = voltage_builtin * cells_in_series - diode_voltage | ||
i_recomb = photocurrent * d2mutau / v_recomb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with
else:
i_recomb, v_recomb = 0, np.inf
instead of defining them first, then potentially redefining them. Seems cleaner, no?
pvlib/singlediode_methods.py
Outdated
""" | ||
# check if need to calculate recombination loss current | ||
i_recomb, v_recomb = 0, np.inf | ||
if d2mutau > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d2mutau : numeric
implies that it can be an array (see here). This if
statement will error if d2mutau
is an array. Perhaps the docstring should be changed to scalar
?
pvlib/singlediode_methods.py
Outdated
grad_i_recomb = grad_2i_recomb = 0 | ||
if d2mutau > 0: | ||
grad_i_recomb = i_recomb / v_recomb | ||
grad_2i_recomb = 2 * grad_i_recomb / v_recomb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, too, I prefer an else statement.
pvlib/singlediode_methods.py
Outdated
v = diode_voltage - i * resistance_series | ||
retval = (i, v, i*v) | ||
if gradients: | ||
# check again if need to calculate recombination loss current gradients | ||
grad_i_recomb = grad_2i_recomb = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No offense, but I don't care for this assignment pattern. Two lines, please.
# Vd = I_sc_ref * R_s | ||
# Id = I_o_ref * (exp(Vd / nNsVt) - 1) | ||
# Ish = Vd / R_sh_ref | ||
PVSYST_FS_495 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dangerous to use a module level dictionary in tests because it can be modified. Suggest turning this into a pytest fixture
@pytest.fixture
def PVSYST_FS_495():
return {...}
probably with a lowercase name but no big deal here.
(5e-4, 0.04)), | ||
(POA, TCELL, {'pmp': 76.26, 'isc': 1.387, 'voc': 79.29}, (1e-3, 1e-3))] | ||
) # DeSoto @(888[W/m**2], 55[degC]) = {Pmp: 72.71, Isc: 1.402, Voc: 75.42) | ||
def test_pvsyst_recombination_loss(poa, temp_cell, expected, tol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if PVSYST_FS_495
becomes a fixture, add it as a parameter so that the value can be accessed in line 161 below.
'poa, temp_cell, expected, tol', | ||
[(PVSYST_FS_495['irrad_ref'], PVSYST_FS_495['temp_ref'], | ||
{'pmp': PVSYST_FS_495['I_mp_ref']*PVSYST_FS_495['V_mp_ref'], | ||
'isc': PVSYST_FS_495['I_sc_ref'], 'voc': PVSYST_FS_495['V_oc_ref']}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall if these dict look ups will work if you turn PVSYST_FS_495 into a fixture. If not, I guess it's ok to leave as is for now.
* 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)
* 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
* 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
Test failure for AppVeyor Python 3.6 due to |
@mikofski sorry I did not realize this was ready. Thanks for making the changes. Looks great. I restarted the build and the tests all pass. |
cells_in_series to calculate i_recomb and v_recomb
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:
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.git diff upstream/master -u -- "*.py" | flake8 --diff
Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):