PVMismatch icon indicating copy to clipboard operation
PVMismatch copied to clipboard

Add De Soto, PVsyst single diode models

Open cwhanse opened this issue 6 years ago • 6 comments

Summary of API breaks:

  • PVcell.Rsh is replaced by PVcell.Rsh_STC. PVcell.Rsh now holds condition-dependent Rsh.
  • PVcell.Eg is replaced by PVcell.Eg_0. PVcell.Eg now holds condition-dependent Eg.
  • Added PVcell.N1_0 and PVcell.N2_0 to contain diode (ideality) factors, which are used in place of the previously hard-coded 1.0 and 2.0 values.

Not yet done: PVcell.Voc still uses diode factors 1.0 and 2.0 for the 2-diode model.

Opening PR to get some feedback on the changes. No tests included.

cwhanse avatar Nov 30 '19 18:11 cwhanse

Hi @cwhanse, I have a few comments, feel free to ignore:

  1. Something similar to your proposal for shunt resistance is nearly already implemented in #91 but I used RSH_E0 instead of RSH_STC and the light dependence is based on the SAM/CEC/DeSoto model

  2. IMHO a more generic approach would be to implement an interface that allows the user to specify the model in pvconst. This was my recommendation in #39. That way the user could specify pvconst= PVconstants(model=pvlib.singlediode.bishop88) and it would use the existing methods from pvlib, rather than reimplenting them here. Your PR seems to be reimplenting the existing pvlib methods and making the existing 2-diode model more generic. While this may be a fine alternate approach, I think it will be more challenging to implement and maintain. That is the approach that I used in the MATLAB pvlife degradation model for SunPower: one model that could be used for 1-diode or 2-diode. There are pros and cons, and I'm okay with either approach ultimately. But in my opinion having an interface that allows the user to swap out the model for any external model will be easier and more flexible.

Related to #115, #39, #91

mikofski avatar Nov 30 '19 19:11 mikofski

Something similar to your proposal for shunt resistance is nearly already implemented in #91 but I used RSH_E0 instead of RSH_STC and the light dependence is based on the SAM/CEC/DeSoto model

The current use of "0", "_0", "0_T0" etc. doesn't follow the patterns I'm used to seeing, which I'm trying to follow. For example, ISC0 is not Isc at STC conditions, it's Isc at reference irradiance only, requiring ISC0_T0 also. I'm hesitant to introduce another variant "E0". In the case of RSH, for the PVsyst one-diode model we need Rsh at STC and Rsh at 0 W/m2. I used RSH_STC and RSH_0 respectively, I suppose RSH_0 and RSH_dark (or something not RSH_0) would work. For the De Soto and CEC models, only Rsh at STC is needed.

IMHO a more generic approach would be to implement an interface that allows the user to specify the model in pvconst. This was my recommendation in #39.

I got the message from @chetan201 that minimal changes to the API are desired, since other applications are relying on PVMismatch. The pvlib functions have a very different API than this package. I would prefer not to duplicate functions already in pvlib, but that's not avoidable without a complete redo of pvcell.

That way the user could specify pvconst= PVconstants(model=pvlib.singlediode.bishop88) and it would use the existing methods from pvlib, rather than reimplenting them here.

I'm confused by this comment: bishop88 is a method to calculate an IV curve given values for the 5 parameters for a single diode equivalent circuit. It's not a single diode model, which in pvlib is implemented in the calcparams functions that intake irradiance and temperature and return the 5 parameters. The counterparts in PVMismatch are pvcell.calcCell (returns the IV curve from parameters) and the family of @property methods (e.g. Isat1, Isat2, etc. Are you suggesting that there's a convenient way to re-use e.g. pvlib.pvsystem.calcparams_desoto here?

But in my opinion having an interface that allows the user to swap out the model for any external model will be easier and more flexible.

I'm in complete agreement, but that's an overhaul of the pvcell API.

cwhanse avatar Dec 02 '19 17:12 cwhanse

@cwhanse thanks for your responses, all sounds good, let me know if/how I can support/help

mikofski avatar Dec 02 '19 19:12 mikofski

Hi @cwhanse, this is really cool! Just catching up on the changes and have started testing a bit.

@mikofski thanks a lot for your continued review support.

Thought I would share this absolute Pmp difference analysis here to hear your thoughts -

image

chetan201 avatar Mar 04 '20 06:03 chetan201

@cwhanse Just curious, do you see this PR getting wrapped up in the next few weeks? I have something I'd like to use it for but can go another way if this is on the back burner. No pressure, I'm just trying to plan ahead.

kandersolar avatar May 05 '20 02:05 kandersolar

@kanderso-nrel I could do that. A few corrections are needed, and tests. I stopped work when the tests became a problem. I cannot exactly reproduce the PVMismatch IV curves using pvlib functions, because PVMismatch parameterizes the curves using Isc rather than photocurrent. The difference in Isc from pvlib and PVMismatch is not small. Let me see if I can find the code to show the problem, I would appreciate someone else's eyes on this. It may be that I'm not using PVMismatch correctly, it also may be that the properties aren't being updated when I expected they are.

cwhanse avatar May 06 '20 17:05 cwhanse