Add PVsyst as option to ModelChain

[cwhanse]

ModelChain currently provides only the De Soto model when a single diode model is used. ModelChain should use a kwarg to select among available single diode models, and test for consistency of the module_parameters dict with the selected single diode model.

[cwhanse]

I need some advice on this one. ModelChain._dc_model can be pointed at ModelChain.singlediode. We need an additional keyword to select among the single diode models so that we call the correct calcparams . Where should we attach this keyword? I'd like to retain the simplicity of dc_model=singlediode and let the module parameters point to the appropriate model. Should ModelChain.infer_dc_model return the additional keyword?

[adriesse]

It occurs to me that PVsyst has a long history, during which it has evolved. So just using the name PVsyst would not be enough to identify an algorithm or model.

I also think we should anticipate that inferring a model from the presence or absence of certain parameters will not always work.

I'm not sure whether that helps you with your specific questions, but it seemed relevant.

[cwhanse]

I'll make certain to note 'v6' in the comments. If we implement a different PVsyst model we can change the keywords.

[markcampanelli]

Please take this with a grain of salt as I myself am still learning about the current ModelChain architecture. ModelChain.singlediode "feels" superfluous to me as an unnecessary/improper layer of wrapping. Recall that PVSystem.singlediode already wraps a "raw" function singlediode, and I think that the scale_voltage_current_power, belongs in the PVSystem wrapper, because it is a system-level property. Is there some way that we can more effectively abstract ModelChain.dc to simply call the appropriate PVSystem calculation, without worrying about if it's a singlediode or SAPM or PVWatts implementation? That said, it seems that a fundamental purpose of ModelChain is to translate weather info into the proper inputs for the dc model, so I am not claiming that ModelChain will be totally agnostic here, rather it's a simplification of the dc abstraction layers.

[wholmgren]

This is tricky.

@cwhanse I agree that we should retain the simplicity of dc_model='singlediode' as an option.

Not sure about the name, but for the sake of the examples linked below, let's say we add a kwarg singlediode_calcparams_model (default None).

The first file in the gist sketches ModelChain wrapper methods for each of the calcparams methods. I don't really like how it turned out. Note that some parts are incomplete because I gave up half way through.

I prefer the second file of the gist. It pushes some wrapping into a PVSystem.get_singlediode_params method. This approach is more consistent with the pvlib approach of implementing get_ methods for wrapping similar functions.

Another related idea, which I haven't fully thought through, is to allow for the possibility of the dc_model keyword doing double duty. For example, dc_model='singlediode' would result in a call to infer_singlediode_calcparams_model, while dc_model='singlediode_desoto' or dc_model='singlediode_pvsyst' would explicitly set the singlediode_calcparams_model attribute.

@thunderfish24 seems to be proposing a larger restructuring of the ModelChain.dc, ModelChain.singlediode, PVSystem.singlediode, and singlediode stack than I am ready to consider this morning, so I will leave it at that for now.

[markcampanelli]

I think there is an even more fundamental disconnect here, which is that the single diode model signature pvsystem.singlediode(photocurrent, saturation_current, resistance_series, resistance_shunt, nNsVth, ivcurve_pnts=None) is not on parity with the other "dc" functions pvsystem.sapm(effective_irradiance, temp_cell, module) and pvsystem.pvwatts_dc(g_poa_effective, temp_cell, pdc0, gamma_pdc, temp_ref=25.).

[wholmgren]

One quick point: ModelChain calls the PVSystem methods, not the pvsystem functions. The method signatures are even more similar:

• PVSystem.sapm(effective_irradiance, temp_cell, **kwargs)
• PVSystem.pvwatts_dc(g_poa_effective, temp_cell)

Note, however, that PVSystem.sapm returns a DataFrame while PVSystem.pvwatts_dc returns a Series (assuming time series input as in ModelChain).

@thunderfish24 are you proposing that we need a method such as:

PVSystem.singlediode_from_weather(effective_irradiance, temp_cell, calcparams='desoto', **kwargs)

If so, I agree that this would simplify the higher level ModelChain code when implementing the PVsyst option.

At the risk of hijacking this issue, some of the discussion in #418 and #83(!) is relevant here.

Edited to add: let's be sure to keep the discussion moving towards addressing the issue of adding a pvsyst option to ModelChain, rather than a larger singlediode refactor unless it's really necessary.

[markcampanelli]

Below is an idea, where the pvsystem.single_diode function is essentially the same as the current pvsystem.singlediode. Note the overloading of F and H vs. effective_irradiance and cell_temp, respectively, in the highest level method calc_model_dc of PVSystem, because if one is using a matched reference cell with a back-of-cell temperature sensor, then F and H are directly available, otherwise F and H would typically be first derived from SAPM's effective irradiance and cell_temp, like the other single diode models. This removes a lot of wrappers in PVSystem, but I haven't totally thought through the SAPM and PVWatts integration at this level or "all the things" into the higher level ModelChain.

class PVSystem(object):
...
    # This property would normally be set by PVSystem constructor
    self.model_dc = 'pvsyst_v6'  # or 'desoto' or 'campanelli' or 'sapm' or 'pvwatts'

    def calc_model_dc(self, overloaded_irradiance, overloaded_temperature):
        if self.model_dc ==  'pvsyst_v6':
            return sdm(get_params_pvsyst_v6(overloaded_irradiance, overloaded_temperature, **self.module_parameters))
        elif self.model_dc ==  'desoto':
            return sdm(get_params_desoto(overloaded_irradiance, overloaded_temperature, **self.module_parameters)
        elif self.model_dc ==  'campanelli':
            return sdm(get_params_campanelli(overloaded_irradiance, overloaded_temperature, **self.module_parameters)
        elif self.model_dc ==  'sapm':
            return sapm(overloaded_irradiance, overloaded_temperature, **self.module_parameters)
        elif self.model_dc ==  'pvwatts':
            return pvwatts_dc(overloaded_irradiance, overloaded_temperature, **self.module_parameters)
        else:
            # Assume custom case is given a function to call
            return self.model_dc(irr, temp, **self.module_parameters)

# These are module-level functions in pvsystem, without wrappers.
def sdm(...):
    # Essentially the same as current singlediode function (not sure about system scaling here)
    ...
    return ...

def get_params_pvsyst_v6(effective_irradiance, cell_temp, **kwargs):
    ...
    return IL, I0, Rs, Rsh, NsVth

def get_params_desoto(effective_irradiance, cell_temp, **kwargs):
    ...
    return IL, I0, Rs, Rsh, NsVth

def get_params_campanelli(F, H, **kwargs):
    ...
    return IL, I0, Rs, Rsh, NsVth

def sapm(self, effective_irradiance, temp_cell, **kwargs)
    ...
    return ...

def pvwatts_dc(g_poa_effective, temp_cell, **kwargs)
    ...
    return ...

[markcampanelli]

@wholmgren Note that I wasn't able to see your reply before posting the above architecture idea.

[markcampanelli]

I think, e.g., get_coeffs_pvsyst instead of get_params_pvsyst_v6 would be closer to Cliff's suggested nomenclature w.r.t. the coefficients of the single diode equation to be solved (with the v6 in the documentation, as mentioned above).

Also, I do value incremental improvements, but I also think the current architecture is a considerably disconnected and has a lot of "glue" :) to wade through when trying to make further contributions.

[markcampanelli]

I added an else case above that handles a custom, user-defined single diode model (now called sdm).

[markcampanelli]

I pushed an initial prototype with more implementation details to #478. WARNING: The code is broken. I am now analyzing the changes to sapm and pvwatts_dc function APIs (inputs and outputs) to make them compatible, and then I will look into the ModelChain integration. I'll point out again that this eliminates a whole bunch of function wrappers in PVSystem (which I see as superfluous), and aims to unify the API for all the DC models in PVSystem, i.e., single diode, sapm, and pvwatts. Another design goal here is to retain modeling workflows relying on "low-level" function calls.

@cwhanse I added a placeholder for where the get_coeffs_pvsyst single diode model coefficients would be calculated.

[markcampanelli]

I think the quickest solution will require yet more glue. Some thoughts for the longer term: (1) If you want people to do something for free, then make the cost to them sufficiently low. (2) Rails Conf 2012 Keynote: Simplicity Matters by Rich Hickey

[cwhanse]

@thunderfish24 Mark I like the direction your pull request is going. It's not a small change and I'm going to have a number of comments/requests, including some to dial back the scope of the changes in some cases. I'm concerned that your (1) remark indicates that we are not communicating well through the comment board. Happy to have a phone call with you, or private emails.

[markcampanelli]

@cwhanse I appreciate you reaching out. At the bottom of it, I think I'm mostly just frustrated that I don't have the time to really offer up working prototype that is integrated across the stack, and I get even more concerned about the time-consuming "heavy lift" involved to (1) properly rally consensus around a ModleChain refactor, (2) actually implement the refactor with the understanding that it may not end up being better and/or accepted in the end, and (3) make for a sufficiently smooth transition for users using legacy code.

I'm taking a little breather here, so I may be quiet for a while. I'll make one last point for consideration on this topic. I am in good agreement with many that the base-level functionality in functions is a key foundation for pvlib. When it comes to a "developer API" for adding DC model functions, etc. into ModelChain or PVSystem, it could be a valuable design goal to make this the same as the "user API" for adding these model functions (e.g., https://pvlib-python.readthedocs.io/en/latest/modelchain.html#User-defined-models). I have seen several software projects where the two API's are different, meaning both have to be understood and maintained and usually one ends up worse off than the other.

[cwhanse]

@thunderfish24 well I appreciate the frustrations about time and about getting consensus on design changes. If you'd like, and there's a practical way to do it, feel free to share your partial code with me (by email or other technique). I think we have reached enough of an agreement on direction of this change that I could push it further along.

[cwhanse]

Closed via #555