Skip to content

Clarify module_parameters['EgRef'] usage, and set a default value #462

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

Closed
cwhanse opened this issue May 15, 2018 · 8 comments
Closed

Clarify module_parameters['EgRef'] usage, and set a default value #462

cwhanse opened this issue May 15, 2018 · 8 comments

Comments

@cwhanse
Copy link
Member

cwhanse commented May 15, 2018

The CEC module parameters database does not contain the parameters EgRef or dEgdT. In the CEC model, and in the process NREL follows to generate the database from the CEC module data list, values EgRef = 1.121 and dEgdT = -0.0002677 are implicit. PVLib for Matlab exposed EgRef and dEgdT as a model parameter (which it is) and let the user determine the appropriate value. pvlib-python does likewise, however, readthedocs.io examples do not inform the user that s/he must specify EgRef and dEgdT nor do we provide default values when they are not specified.

I propose that we:

  1. assign default values within pvsystem.retrieve_sam('cecmod') and
  2. add to the example at http://pvlib-python.readthedocs.io/en/latest/modelchain.html to illustrate use of the CEC module model, and the opportunity to specify EgRef and dEgdT, if desired.
@wholmgren
Copy link
Member

Sounds good to me.

@adriesse
Copy link
Member

Would it make sense to lobby NREL to put those parameters into their database?

@cwhanse
Copy link
Member Author

cwhanse commented May 16, 2018

It would. I opened an Issue on the SAM GitHub.

@cwhanse
Copy link
Member Author

cwhanse commented May 16, 2018

Changing the plan a bit.

pvsystem.retrieve_sam('cecmod') should only read and return the content of the CEC module parameter database. I don't think it's a good idea to create and populate new fields in this function.

However, currently a PVSystem object expects module_parameters to contain fields EgRef and dEgdT because it passes self.module_parameters['EgRef'] as an argument from method calcparams_desoto to function calcparams_desoto

I don't want to add a new kwarg to method calcparams_desoto, the signature is nice and clean as is. But we want to be able to pass values to function calcparams_desoto if the user has specified values.

So, what does this group think:

  • Add code to PVSystem.calcparams_desoto to first check if EgRef and dEgdT exist, and if they do, to pass a kwarg to function calcparams_desoto.
  • Function calcparams_desoto assigns default values if no kwarg is passed.

My hesitation is not knowing if adding code into the method which simply wraps an external function is breaking a style or programming convention.

@wholmgren
Copy link
Member

If I understand your proposal correctly, it doesn't break any conventions. We avoid setting any parameters in the wrapper methods, but we do have some code to gracefully mesh method calls with function keyword arguments. See, for example, PVSystem.ashraeiam and PVSystem.physicaliam.

So, we can:

  1. Change function calcparams_desoto signature to include EgRef = 1.121 and dEgdT = -0.0002677, then
  2. Add _build_kwargs call within PVSystem.calcparams_desoto.

Open question in my mind as to whether or not step 1 is an appropriate change. I lean towards yes. And yes, there are alternative ways to accomplish the kwarg meshing.

Related but maybe separate... this reminds me that the calcparams_desoto function has a weird signature. Might be worth rethinking the signature separate from the constraints of the NREL/CEC database. Perhaps every module/cell parameter should be an argument or keyword argument, or every parameter should be wrapped up into a module_parameters dict (as in PVSystem.calcparams_desoto).

@cwhanse
Copy link
Member Author

cwhanse commented May 16, 2018

I agree about the calcparams_desoto signature being awkward. It's a direct transcription of the PVLib Matlab implementation which passed individual parameters and others bundled in a structure. I'd prefer to see all parameters passed as individual keywords. If we go with all keywords then we use PVSystem.calcparams_desoto to unpack the dict via _build_kwargs (thanks for pointing out this function!) The most common use of calcparams_desoto is probably via PVSystem.calcparams_desoto where the module_parameters dict is very convenient.

@wholmgren wholmgren added this to the 0.6.0 milestone May 16, 2018
@adriesse
Copy link
Member

While you're at it, I would suggest removing the parameter M. It's really a generic mismatch factor, which is perhaps better implemented externally to modify effective irradiance. It doesn't actually become an "airmass modifier" until you use airmass to determine M. I'm not sure where the proposed polynomial should be placed thought, since the other two spectral correction functions (FS and SAPM) are organized in different ways. I don't know what the best solution is but I just wanted to draw your attention to this.

@cwhanse
Copy link
Member Author

cwhanse commented May 31, 2018

@adriesse I agree. pvsystem.calcparams_desoto should expect effective irradiance as its input, not broadband-plane-of-array-after-reflection-losses which is what 'S' means in Desoto's paper. We preserved that notation and usage in PVLib-matlab, but I don't think its necessary here. I would prefer the calcparams_xxxxx family of functions to have a common signature, and expect the same quantities as input as pvsystem.sapm.

However, strictly speaking, substituting effective_irradiance for 'S' changes the Desoto model from its published reference. Desoto wrote that Rsh = Rsh_ref * (1000 / S), not Rsh = Rsh_ref * (1 / effective_irradiance). The difference is whether spectral change in irradiance is accounted for before, or after, shunt resistance is calculated. From a practical perspective, I doubt the distinction is meaningful for most uses of the Desoto model, and I suspect the authors didn't think this is an important point. I'm going to change to effective irradiance and document the change with inline comments.

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

3 participants