Open markcampanelli opened 6 years ago
@thunderfish24 if I am following your ideas, you are proposing to replace the current calcparams_<modelname>
set of functions with a wrapper calcparams_hybrid
, which returns the 5 values for the single diode equation, but uses kwargs
to select particular auxiliary equations, e.g., nNsVth_desoto
, nNsVth_pvsyst
, etc. Is that correct?
If we could streamline the kwarg
to select a set of auxiliary equations (e.g., pass desoto
once to get the Desoto model) I could see this as a significant improvement in the API.
I wonder how we'd deal with the necessary model parameters which are different among, and specific to, each single diode model (e.g., Desoto has only Rsh_ref
whereas PVsyst has 3 parameters in the equation for Rsh
, CEC model has a unique parameter Adjust
.).
I like the idea of a wrapper function as we implement more calcparams_*
functions. I'm afraid that I don't quite follow the logic in its entirety. PR #427 implements a couple of new get_
functions that provide a unified interface to underlying functions. Could that more explicit approach be taken here, too, or am I missing something?
@cwhanse @wholmgren I am not necessarily proposing to replace all the calcparams_<modelname>
functions with a single wrapper. Some sufficiently similar models may be consolidated (perhaps, desoto, pvsyst, and cec can be a "family"?), but, as Cliff observed, I think that there may be sufficient differences between some models that combining them all may become quite opaque/complicated (e.g., my F
vs. SAPM's E_e
). I definitely see opportunity for this approach to make swapping in different auxiliary equations easier within a given model family, e.g., constant Rs
vs. Kyumin's temperature-dependent model here. Of course, I also have an eye towards automated optimal model selection from a dataset using multiple "flavors" of a model, and then pvlib models can more easily implement the various flavors.
As Cliff also points out, I think the assembly of the corresponding **kwargs
would be hidden from the "typical" user operating at a higher level (e.g., the ModelChain), and there would be some method of high-level configuration that hides the implementation details. (I'm looking for ideas here!) A "power user" however would find a lot of flexibility under the hood, at the expense of less overt function calls. For example, a user should be able to swap in a user-defined auxiliary equation "on the fly", if they know what they're doing. Also, the design pattern might readily mature into a more powerful computational graph method such as Dask's delayed.
So to be clear, the word "hybrid" in calc_params_hybrid
refers to a hybrid of the Campanelli et al. model and the SAPM.
I'll admit I'm intrigued with the idea that we can wrap the calcparams and specify the model with a keyword. I think the challenge will be to manage the model parameters. There's no getting around the fact that each model has some unique values, and that the parameter sets we have to offer are only for one of the diode models (the CEC model).
I think the flexibility to mix and match auxiliary equations has value but not to most users. There is value is our current API for most users. Using separate functions for each model forces us to be explicit and transparent about the equations are used in each model - and these are primary objectives of pvlib.
So despite the intriguing nature of the wrapper, I'm hesitant to say let's replace the 'one calcparams per model' approach.
For the 'hybrid' model you propose, I'm ok with that as long as there is a published reference documenting the model being submitted.
Based on the development in docs/tutorials/pvsystem.ipynb
, I scaled back my original inclusion of the control variables F
and H
in kwargs
. Computing these separately, when needed, seems to make more sense in practice. This means the model is no longer the hybrid that I originally envisioned. You can find how this model is executed using the SAPM for the F
and H
computations in my additions to docs/tutorials/pvsystem.ipynb
. Recall that a dataset from a matched reference cell with a junction temperature sensor, for example, would have F and H directly available.
I'll echo Cliff's comments:
Using separate functions for each model forces us to be explicit and transparent about the equations are used in each model - and these are primary objectives of pvlib.
The **kwargs approach outlined here is a totally valid and very powerful for some use cases. But I am having a hard time reconciling it with the approach taken by pvlib so far.
For example, a user should be able to swap in a user-defined auxiliary equation "on the fly", if they know what they're doing.
ModelChain
supports this while retaining fairly explicit code. Could a similar approach be taken here?
I don't really understand the connection to dask/dask.delayed. Those are flexible frameworks and, in my experience, work fine with the existing pvlib api. On a related note, my experience with using dask/dask.delayed is that it's even more important to use very explicit code in those frameworks because of the additional debugging challenges they introduce.
@wholmgren I am now familiarizing myself with the ModelChain as I evaluate architecture options for adding a new flavor of diode model to pvlib. Is the statement about ModelChain here still valid: "CEC module specifications and the single diode model are not yet supported."?
The Dask delayed
connection is merely a possible design goal to keep in mind, and I do sense that the kwargs
approach may be harder to support in this respect. I tend to prefer declarative syntax (e.g., the kwdargs
dict) where the implementation is automated in the sense that a user wouldn't have to explicitly re-code the computational graph/ordering when dependencies among the auxiliary equations change. My current solution is declarative, but is not flexible on the automated implementation. Granted, this particular SDM's auxiliary equations have constraints and limited options that might make my point somewhat moot (e.g., the IL is always the last auxiliary equation computed, because it depends on all the others, and after IL only I0 depends on any other auxiliary equation). Nevertheless, I'd still like to learn about the architecture options here.
@wholmgren I think it might be wise to open a separate issue to determine how we are going to implement the various single diode models such as pvsyst, first_solar, and campanelli so that they all integrate into ModelChain
. This code is tightly coupled to the DeSoto model, and so I'm not sure if we are expected to somehow implement these in custom PVSystem
objects that are wrapped by the ModelChain
. (My model, also uses F
and H
instead of effective_irradiance
and temps
, and the F
is not simply a normalized version of the existing effective_irradiance
.)
We need a singlediode_model_setter
and a _infer_single_diode_model
that operates on the parameter keywords.
If I understand correctly, I think the standard pvlib approach would work and I don't think we'd need custom PVSystem
objects. The standard approach being:
PVSystem
wrapper method that removes all of the unchanging stuff relating to module parameters, etc. ModelChain
methods that call PVSystem
methods with the appropriate data. ModelChain
methods that inspect the properties of the supplied PVSystem
object to automatically choose the model parameters.We would want to decouple ModelChain.singlediode
from the DeSoto model.
By custom PVSystem
objects do you mean that you don't agree that we need a new singlediode_model_setter
? Rather, we could extend the keywords in the current ModelChain.dc_model
to include 'desoto', 'pvsyst', 'CEC', etc., with 'singlediode' defaulting to 'CEC' in my judgment.
ModelChain.singlediode
needs a new kwarg for the different single diode models.
I agree that the singlediode_kwarg = _infer_single_diode_model(module_parameters)
is optional. We can embed the error testing (do the parameter exist for the single diode model specified) in the model setter.
I would like to implement a hybrid model that combines the single-diode model (SDM) developments of Campanelli et al. (see this) with the SAPM to compute the effective irradiance ratio F and effective temperature ratio H using the SAPM when necessary, i.e., when not using a matched reference device or cell-embedded junction temperature measurement.
I figure one should embrace Python's duck typing (Quack!), so below is my proposed SDM 5 coefficient calculation architecture that is a partial generalization of the existing function
calcparams_desoto
. Basically, this is a way of implementing a (somewhat inelegant) computational graph for all the coefficients (e.g., IL, I0, Rs, Rsh, nNsVth). I realize many folks aren't familiar with my model, but if you work through this, you'll see that something like this pattern may make it easier for users to implement different "flavors" of auxiliary equations for the SDM when they are trying to compare models, find a better model, and the like. (I may need to recast my model in terms ofeffective_irradiance
andcell_temp
to make it fit better with the current direction of the pvlib API, and this might make possible more reuse of auxiliary equations between different SDMs.)There is a lot of metadata packed into
kwargs
, and this provided metadata would change with the particular auxiliary equations selected. (I apologize that this will be a bit hard to understand from just the code snippet below, but I am still working on a runnable example.) What's nice about this pattern, is that one can specify a scalar, an array, or a function-to-be-computed for F, H, and IL, I0, Rs, Rsh, nNsVth, and once a parameter is computed, it can then "flow through" to the remaining computations. (However, unlike Dask, the user has to specify the computation in the proper (serial) order to avoid missing computations or re-computations of the computational graph's nodes.)