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

Adding New Single Diode Model For CdTe #163

Closed
crimmj opened this issue May 10, 2016 · 27 comments · Fixed by #504
Closed

Adding New Single Diode Model For CdTe #163

crimmj opened this issue May 10, 2016 · 27 comments · Fixed by #504
Milestone

Comments

@crimmj
Copy link

crimmj commented May 10, 2016

Would like to add a new diode model(s) appropriate to non c-Si modules. There are some additional parameters in the model(s). Any thoughts on the correct structured way to do this? Would this function or functions be added to pvsystem.py? Do modules need to be a class if they have different appropriate performance models?

@wholmgren
Copy link
Member

Sounds useful to me. The existing functions that use module parameters will work with anything that supports dictionary-like lookups. Most of the time, people will be passing in Series objects from the SAM DataFrames, but plain dictionaries should work too. I would say that we should keep things simple and use a similar interface for new functions.

@wholmgren wholmgren added this to the Someday milestone May 11, 2016
@cwhanse
Copy link
Member

cwhanse commented Jun 11, 2018

This request essentially asks to implement the PVsyst model for a-Si, which includes a new term to account for recombination loss in the intrinsic layer (see http://files.pvsyst.com/help/thinfilm_recombinationloss.htm

PVsyst provides a default value for the single new parameter (di^2/mu teff = 1.4V).

Implementing this new model depends on having calcparams_PVsyst see #470 and will require a new function to solve the diode equation with the recombination current term.

@mikofski
Copy link
Member

I have this model already, I can try to add this

@cwhanse
Copy link
Member

cwhanse commented Jun 12, 2018

@mikofski happy to have a volunteer! Are you thinking to implement the PVsyst model (addressing both #470 and #163) or only the variation of singlediode to solve the current-voltage equation? If you need help with #470 I can do the calcparams_pvsyst component.

@mikofski
Copy link
Member

mikofski commented Jun 17, 2018

If the recombination loss current is already included in the MATLAB version, then I guess we should just port that over here. @cwhanse are you planning to do that or are you looking for volunteers?

@cwhanse
Copy link
Member

cwhanse commented Jun 17, 2018

I have some MATLAB code which isn't in PVLib. I didn't find an elegant solution, it's brute force getting the current at each voltage.

@mikofski
Copy link
Member

mikofski commented Jun 17, 2018

Okay let's collaborate then. I use the same explicit approach for PVSyst as I do for any SDM. Then either interpolate or Newton method or bisection for specific points or the max p. I'll try to put something up this week. I think this counts as work don't you? On a related note, I added an issue on SAM SSC on the recombination current for thin film. I think it should be added to the CEC and DeSoto models, the J. Merten, Mermoud, and Lejeune seem to indicate that an error would be present with thin film for any SDM near Voc missing this term. Do you have an opinion? Maybe we can collaborate with FirstSolar.

@cwhanse
Copy link
Member

cwhanse commented Jun 18, 2018

I think I need to be clear on the plan here. This issue asks for a new function (counterpart to pvsystem.singlediode that solves

I = IL - IL x mu/[Vbi - (V+I x Rs)] - I0 x [exp((V+I x Rs)/(nNsVth))-1] - (V + I x Rs)/Rsh

returning the IV curve. The second term on the right is the recombination current.

#470 asks for calcparams_PVsyst which is related, but basically independent, because the parameters unique to the recombination term (mu, Vbi) are constants.

We can work together to implement singlediode_recomb or whatever name we settle on. I'll email my Matlab function to you.

@mikofski
Copy link
Member

Yes, I'm clear. What I was asking about was that since the only difference between the existing single diode model (SDM) and a model that included the thin film recombination loss current is the term:

I_rec = I_L * d2_mu_tau / (V_bi - V_diode)

then:

  • does it really make sense to create a separate sdm_thin_film method that is nearly identical to the existing SDM method?
  • would it be possible to just include I_rec = I_L * d2_mu_tau / (V_bi - V_diode) into the existing pvsystem.singlediode and just turn it on as needed?

As I said previously, the solution for either is exactly the same: specify a bunch of diode voltages, solve for cell current then back calculate the cell voltage. For specific voltages and max power, then you can either interpolate, use Newton, or use a bisection search method. (Maybe Lambert-W also works but I'm not as familiar with that formulation?)

I understand that calcparams_pvsyst merely generates the 5 parameters that SDM uses since PVSyst has different auxiliary equations, so I understand that's the focus of #470.

So, as I asked above, for #163 we could either implement a new SDM method for thin film with recombination losses or just combine it with the existing SDM.

Regarding:

I think it should be added to the CEC and DeSoto models, the J. Merten, Mermoud, and Lejeune seem to indicate that an error would be present with thin film for any SDM near Voc missing this term. Do you have an opinion? Maybe we can collaborate with FirstSolar.

Sorry to be so vague with that comment. What I meant was the question I asked above: shouldn't we just include the recombination loss current into the existing pvsystem.singlediode such that it would be available to any calcparam_*() method, not just PVSyst? I was thinking that by collaborating with FirstSolar we could determine whether or not the existing SDM without the recombination loss current exhibits the errors that J. Merten, Mermoud, and Lejeune observed.

@adriesse
Copy link
Member

I think it makes sense to incorporate this feature into the SDM functions. The various models can then use the feature as needed. Maybe it even opens up some unexplored territory.

@cwhanse
Copy link
Member

cwhanse commented Jun 19, 2018

I'm tracking with you now @mikofski If we use the diode voltage method to solve the IV curve this new term is not a challenge. But singlediode and its helpers, i_from_v and v_from_i are using the LambertW method which doesn't extend when the recombination current is added to the single diode equation.

We had a long conversation about changing to the diode voltage approach #409 which hasn't been merged. We need to resolve that before moving ahead with this issue.

Since it has been a while, @mikofski can you remind us what issues remain with #409?

@mikofski
Copy link
Member

mikofski commented Jun 21, 2018

I've started trying to update #409, there's a list of TODO[s] that @wholmgren left for me to check off like fixing docstrings, better names (newton vs. "fast", etc.) and moving vendorized scipy.optimized.newton to _tools since it won't be merged until November (if ever ☹️ )

@mikofski
Copy link
Member

mikofski commented Jun 28, 2018

TODO: see this comment from #409 to add Irecomb to bishop88

Yes. Set d2mutau=0.0 and Vbi=0.9, user can change the values with kwargs if wanted.

@mikofski
Copy link
Member

TL;DR:

Should we always limit IV curves for CdTe (and a:Si?) to the 1st quadrant only, or to a voltage in the 4th quadrant that is very close to open circuit voltage but much less than the intrinsic voltage?

More Discussion:

@cwhanse as voltage approaches the builtin voltage, the expression for the recombination loss current asymptotes to infinity:

diode_voltage = voltage + current * series_resistance
Irecombination = photocurrent * d2mutau / (intrinsic_voltage - diode_voltage)

@jdnewmil has suggested gradually turning off the recombination somewhere between open circuit (V_oc) and the intrinsic voltage of the thin layer, which should always be greater than V_oc.

There is nothing in Meromoud's Performance Assessment ... that suggests what to do.

Do you have any suggestions?

First to be clear, the recombination loss error, does not affect all thin films. For example, CIS/CIGS are not affected. Merten et al and Mermoud et al only observed the recombination loss for amorphous silicon (a:Si) and cadmium telluride (CdTe).

My idea would be to just limit IV curves for CdTe (and a:Si?) to the 1st quadrant only. This avoids the problem. There's no reason to go into the 4th quadrant for thin film because they typically don't have bypass diodes and therefore will fail badly if there is any partial non-uniform shade, ie: only row-to-row shade that effects the entire array (except the front row) evenly. Also most CdTe (and a:Si?) are typically only installed on trackers, and are only shading when the sun is low in the sky since most CdTe (and a:Si?) do not backtrack. Since the trackers are aligned N-S, the sun is nearly due east or west during sunrise/sunset, and the irradiance is very low. String voltage mismatch might be an issue at sunrise/sunset because of edge effects since the sun is not exactly due east or west, but the voltages should all be well below the intrinsic voltage due to the low irradiance, and voltage mismatch would be small because the sun is nearly due east or west.

Thanks for your comments.

@cwhanse
Copy link
Member

cwhanse commented Jul 11, 2018

Yes, 1st quadrant only is fine from my point of view.

@jdnewmil
Copy link

jdnewmil commented Jul 12, 2018 via email

@cwhanse
Copy link
Member

cwhanse commented Jul 12, 2018

@jdnewmil your use case is of interest to me also. The model we are implementing is chosen because its being used in PVsyst. I don't know if PVsyst does calculations outside the 1st quadrant.

I worry about applying this model at high external voltage. The way I read the reference is that this particular model for the recombination current loss derives from some assumptions that are valid at low external voltage, but questionable as the system moves beyond Voc. I'm not eager to spend effort to make this particular calculation reliable as diode voltage approaches the intrinsic voltage parameter value. I think we'd be better served looking for a modeling approach which could be justified at these voltages, e.g., Augusto et al (207) Analysis of the recombination mechanisms of a silicon solar cell with low bandgap-voltage offset.

@mikofski
Copy link
Member

Proposed changes as discussed in #409

What are good names for the new arguments?

  • intrinsic_voltage? builtin_voltage? thin_film_voltage? some combination?
  • diffusion_distance_ratio? thin_film_cross_section? instrinsic_recombination_rate? or should we just stick to d2mutau? or call it merten_et_al_factor?

remaining questions:

  • should users be allowed to set Vbi? What about tandem and triple junctions? Should we add a parameter for number of junctions, or add a note in the docstring or elsewhere for the user to adjust Vbi as needed?

I'll submit a PR soon, but it'd be nice to get some feedback on suggested names, any mistakes in my derivatives, or ideas about arguments and code paradigms. Thanks!

@cwhanse
Copy link
Member

cwhanse commented Jul 13, 2018

voltage_builtin I think . I prefer to put the quantity first. The Mertens paper references earlier work that uses the term 'built-in voltage' and that term seems to have stuck. 'intrinsic voltage' appears to be a PVsyst term. Users should be permitted to set voltage_builtin via a kwarg but we should supply 0.9V as the default.

We could consider adding a warning that this model is only considered appropriate for amorphous silicon cells. Validity at high forward voltage is a different issue...

@mikofski
Copy link
Member

... and CdTe, but not CIGS - Yes I agree, this should be emphasized. What to call d2mutau ? My vote is to leave it as is.

@jdnewmil
Copy link

jdnewmil commented Jul 13, 2018 via email

mikofski added a commit to mikofski/pvlib-python that referenced this issue Jul 14, 2018
* 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
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
@adriesse
Copy link
Member

adriesse commented Aug 8, 2019

Not sure it is pythonic (or gitic) to comment on a closed issues, but it seems it would be really useful to have d2mutau and NsVbi accepted in bishop88_mpp, or something like that. Has anyone got that on their radar screens?

@cwhanse
Copy link
Member

cwhanse commented Aug 8, 2019

Not on my radar. I agree it would be useful. Let's open a new issue for the enhancement request.

@mikofski
Copy link
Member

mikofski commented Aug 8, 2019

OK, but bishop88 does already take these arguments:

def bishop88(diode_voltage, photocurrent, saturation_current,
resistance_series, resistance_shunt, nNsVth, d2mutau=0,
NsVbi=np.Inf, gradients=False):

Seems like an oversight that they're not in bishop88_mpp, bishop88_i_from_v, or bishop88_i_from_v. I believe this may be an easy fix?

@adriesse
Copy link
Member

adriesse commented Aug 9, 2019

Seemed like something that fell between the cracks. Is it really just a matter of passing the parameters through?

@mikofski
Copy link
Member

mikofski commented Aug 9, 2019

yes

@adriesse
Copy link
Member

adriesse commented Aug 9, 2019

@mikofski Care to look in my bishop branch? It's all I had time for today; I think it works. Maybe a PR next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants