-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Initial Campanelli et al. SDM #478
base: main
Are you sure you want to change the base?
Conversation
TBH, I'm not 100% sold on using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick comments, I haven't looked this over in detail:
calc_params_campanelli
orcalcparams_campanelli
The pattern (so far) has beencalcparams
which I'm not particularly fond of, butcalc_params
doesn't seem better.- I'd make the various
get
functions private, they are (currently) applicable only to the campenelli model.
@@ -279,7 +326,8 @@ def physicaliam(self, aoi): | |||
|
|||
return physicaliam(aoi, **kwargs) | |||
|
|||
def calcparams_desoto(self, effective_irradiance, temp_cell, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any kwargs passed here are overwritten.
pvlib/pvsystem.py
Outdated
return sdm(get_coeffs_campanelli(irr, temp, | ||
**self.module_parameters), | ||
modules_per_string=self.modules_per_string, | ||
strings_per_inverter=self.strings_per_inverter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm..., I think I should be more consistent here and make sdm
have the same API/signature as sapm
and pvwatts_dc
. This might mean that the get_coeff_*
call is determined somehow from the module_parameters
and done inside sdm
.
pvlib/pvsystem.py
Outdated
**kwargs): | ||
|
||
self.name = name | ||
self.model_dc = model_dc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possibility here is to specify model_dc
directly as a callable in both the "built in" pvlib model cases and the user-defined model case, instead of a string or a callable, respectively. I like this idea, but it may seem more opaque to some users.
I should have some new code addressing my above comments coming soon, including a common API for pvwatts, sapm, and various single diode models, as well as assigning functions instead of strings to |
@wholmgren I have decided to try to implement my model in the existing
Is there a "standard workflow" example of |
@wholmgren Nevermind. I see that workflow example now earlier in |
@wholmgren I pushed up some code with an initial attempt at Would you mind taking a look, both at my integration approach and what might be causing this particular error? |
@wholmgren Got that sorted (facepalm). Working too late the other night. |
I now have a working example in |
I have added working examples in of both types of "wrappers" for the Campanelli et al. model that work with I also simplified the model implementation code for this first implementation, and I hit a problem running the SAPM in ---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-22-30449faf83de> in <module>()
8 am_abs, aoi, module)
9
---> 10 sapm_1 = pvlib.pvsystem.sapm(effective_irradiance, temps['temp_cell'], **module)
11
12 sapm_1.plot()
TypeError: sapm() got an unexpected keyword argument 'Vintage' @wholmgren @cwhanse I am requesting a review of this code, because the changes might surprise some people and there might be better ideas out there on how to implement this new type of weather data. |
I could easily be wrong here. You are passing Suggest simply passing |
pvlib/modelchain.py
Outdated
@@ -803,7 +803,7 @@ def prepare_inputs(self, times=None, weather=None): | |||
self.weather['temp_air'] = 20 | |||
return self | |||
|
|||
def run_model(self, times=None, weather=None): | |||
def run_model(self, times=None, weather=None, pv_ref_device=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opposed to this new keyword argument. Measured F
and H
should be loaded into weather
by the user. Calculation of F
and H
should be part of sdm_campanelli
for now; I think these calculations have longer-term value but I don't want to hold up a pull request and require that work now. I suggest we open bite-size issues to move the F
and H
functionality to more general positions. They are likely of use for sapm
, and need to be deconflicted from other functions/quantities we term effective_irradiance
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I'm looking into the alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to suggestions on what to call F
, as well as the symbol. Perhaps something more expressive (but wordy!) like "short-circuit current ratio". Likewise for H
, but note that the formulation of this model in terms of H
is more of a numerical convenience (a re-scaling that also obviates the need to explicitly specify the reference temperature), whereas F
is fundamentally linked to an "observable" short-circuit current rather than a "non-observable" photocurrent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something more wordy would be good. Ideas:
effective_current_ratio, i_sc_ratio, i_sc_norm
temp_cell_abs_norm, temp_cell_norm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
effective_irradiance_from_isc
perhaps. temp_cell_normalized
is ok with me.
@@ -279,7 +280,7 @@ def physicaliam(self, aoi): | |||
|
|||
return physicaliam(aoi, **kwargs) | |||
|
|||
def calcparams_desoto(self, effective_irradiance, temp_cell, **kwargs): | |||
def calcparams_desoto(self, effective_irradiance, temp_cell): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem. Method calcparams_desoto
needs the **kwargs
because it unpacks these arguments to pass them to function pvsystem.calcparams_desoto
as positional arguments.
I don't understand why you are removing **kwargs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 302 below assigns directly to kwargs, which (as far as I can tell) clobbers any value for the kwargs dict passed to this method.
Actually, the situation appears similar in the sapm method too, but there the passed kwargs dict is simply never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To followup further, would you be opposed to forgoing the _build_kwargs()
call in the calcparams_desoto
method and using the following (more direct) return statement instead?
return calcparams_desoto(effective_irradiance, temp_cell, **self.module_parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the _build_kwargs(...)
step doesn't work given the current setup of calcparams_desoto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need _build_kwargs
for the function call. But I think we can get rid of the kwargs
in the method signature above. Would need to be documented in API changes.
currents = ['i_mp', 'i_x', 'i_xx', 'i_sc'] | ||
data[voltages] *= voltage | ||
data[currents] *= current | ||
data['v_mp'] *= voltage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these improvements.
Update: (facepalm) The gremlins were all in my head apparently! I fixed the changes I made that broke cells in |
At the risk of a small bit of scope creep, I pushed up some simplifications to the |
So based on the test failures, the |
Travis-CI 3.6 build passed. All other failures appear to be spurious and unrelated to these changes. |
@cwhanse Please review my updates. I removed the Note that there is still logic required in I also improved the accuracy/consistency of some of the docstrings in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thunderfish24 I made a quick, non-comprehensive read through with comments below. It's looking good!
self.dc_model() | ||
self.ac_model() | ||
self.losses_model() | ||
|
||
return self | ||
|
||
|
||
def sdm_campanelli(mc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting idea to make a function here rather than a ModelChain method. It doesn't follow the pattern, but I could be convinced that all of the wrappers should look like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the current architecture where the PVSystem
wrappers are not ultimately responsible for determining the actual computations in a performance simulation, this is the "shortest path" I could come up with ;). It also pushes the idea that the "user" API could be the same as the "developer" API. (I of course, think that it should be the same ;).)
effective_irradiance.values / irrad_ref | ||
|
||
# Model the effective temperature ratio H from SAPM temp_cell | ||
H = (temps['temp_cell'].values + 273.15) / (temp_ref + 273.15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why .values
? might need a comment if we decide to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, it is to ensure the "raw" numpy data structure in order to satisfy the (simplified) API of the low level function in the pvsystem
module. (Of course, I haven't documented that yet.)
@@ -279,7 +280,7 @@ def physicaliam(self, aoi): | |||
|
|||
return physicaliam(aoi, **kwargs) | |||
|
|||
def calcparams_desoto(self, effective_irradiance, temp_cell, **kwargs): | |||
def calcparams_desoto(self, effective_irradiance, temp_cell): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need _build_kwargs
for the function call. But I think we can get rid of the kwargs
in the method signature above. Would need to be documented in API changes.
Absolute airmass. | ||
|
||
aoi : Series | ||
Angle of incidence (degrees). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all this related clean up.
@@ -574,7 +566,7 @@ def scale_voltage_current_power(self, data): | |||
|
|||
Returns | |||
------- | |||
scaled_data: DataFrame | |||
scaled_data: DataFrame or (ordered) dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to add dict
, but no need to specify ordered. I think we'll be getting rid of OrderedDict as python 3.6+ becomes the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we've used dict-like
elsewhere and it applies here as well. so that's another option.
return singlediode(*get_coeffs_campanelli(F, H, **kwargs)) | ||
|
||
|
||
def get_coeffs_campanelli(F, H, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use **kwargs
here? Would we not want to use positional arguments for everything that's required and does not have a default, and use named keyword arguments for everything that has a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless someone can show that this is less performant, I think it is less fragile and thus preferable to _build_kwargs
. Of course, the required kwargs
must be well-documented in the "base" function. Given the considerable level of indirect inference and "kwargs packing" that occurs throughout the current architecture, I don't find this any more obscure than the status quo. I realize some might disagree here and prefer positional arguments, but I personally really prefer removing order dependence whenever possible in function arguments (esp. for model parameters like IL
as opposed to control variables like effective_irradiance
). On a related note, Python3 has keyword-only arguments that don't have to specify a default value :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python makes no distinction. The user can provide keyword or positional arguments whenever they want unless the function signature contains splat operators like *a, **kw
, which are only used to match inherited signatures, or to collect unknown user supplied extra arguments.
import this
says explicit is better than implicit. IMHO unless you are collecting args for inherited signatures or allowing the user to supply their own keyword arguments, this usage should be avoided, and I believe most linters would warn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K. So I am going to look up if there is a Python2 backport of keyword-only arguments that don't require a default value. I think this would be a fair compromise to my preference to avoid the complexity of order while also being uber-explicit in the low-level functions.
The only downside that I see here is potentially long keyword names, but again I think explicitness and "self-documenting" function readability trumps lengthiness, and think my names are short but sufficiently expressive and will of course be fully documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that @thunderfish24 and others will think this a step backwards, but I'd prefer calcparams_campanelli
to get_coeffs_campanelli
to keep the function name pattern parallel to the other version of single diode models. When we want to change this API pattern we can do it for all single diode model coefficient functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No prob. Consistency is always an important consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @cwhanse that calcparams_campanelli
is preferable here. My intention (perhaps poorly communicated elsewhere) is for all get_*
functions/methods to be wrappers for other functions/methods. See #427 for more on that pattern. This PR is big enough already, so let's not add a new wrapper here.
Re keyword-only arguments... I am not aware of a way to do that in Python 2 without some miserable hacks. Users can still call positional arguments as keyword arguments in any order they would like, but I think it's good to be consistent within pvlib itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that Python2 hacks look really bad, so I'll probably have to give up and simply used named positional arguments :(.
# Clear any stale value | ||
if hasattr(self, 'F'): | ||
print('F cleared') | ||
del self.F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is debugging code, but we don't do this for any other modelchain attributes and I think we should avoid adding this layer of complexity in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I forgot to remove the print statements, but in general these do need to be cleared to prevent subtle bugs that the user could run into when re-running a model with changing weather types, because I think it is generally valuable to "record" the F
and H
values used in the ModelChain
object, which sometimes come from the "non-trivial" SAPM model calculations. (This is not too different than other "recorded" outputs, such as effective_irradiance
.)
|
||
# Calculate the dc power and assign it to mc.dc | ||
mc.dc = pvsystem.sdm_campanelli(mc.F, mc.H, **mc.system.module_parameters) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I have forgotten to scale the output for modules_per_string
and strings_per_inverter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can sdm_campanelli_sapm
be built into sdm_campanelli
? The _sapm
variation only fills in F
and H
when they are not supplied as weather data.
del self.H | ||
if not hasattr(self, 'F') and not hasattr(self, 'H') or \ | ||
hasattr(self, 'F') and not hasattr(self, 'H') or \ | ||
not hasattr(self, 'F') and hasattr(self, 'H'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can anyone figure out a simplified equivalent logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False only when both F
and H
are present: not (hasattr(self. 'F') and hasattr(self, 'H'))
Personally, I don't see why this added branching logic in run_model
for a specific dc_model
choice is needed. When dc_model
is set to sdm_campanelli
, then sdm_campanelli
can test for sufficiency of its inputs, and if F
and/or H
are missing, then sdm_campanelli
can calculate them using the sapm
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A whole set of functions do not need to be run in some cases, and I think they fail in the case where there is no data for them to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points, but I think prepare_inputs
should be protection against missing values - you can even pass weather=None
and it will fill in with a clear-sky model. I realize that sdm_campanelli
doesn't need anything upstream of dc_model
but I'd prefer to see the model chain sequence maintained.
My instinct is that an if...then
structure to skip model chain steps for a choice of a downstream model will be a problem to maintain, and will become difficult to manage as more models are added.
I would prefer a separate pull request to extend the ModelChain
functions to handle measured effective_irradiance
and temp_module
as inputs in a general manner. I think this pull request should conclude when sdm_campanelli
works as a method of the current ModelChain
, with supporting functions consistent with the current patterns. I'm OK with less efficient code in favor of consistency. Redesign of ModelChain
and generalizing treatment of input options can be opened as their own issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a more elegant solution most likely lies in an architecture refactor.
What I really want to avoid here is having the user have to specify a bunch of SAPM parameters in the cases where they are not needed and indeed may not even be available. (That would be onerous, confusing, and quite out of the spirit of the matched-reference-device approach.) Assuming there are no more surprises, I'll probably settle for hacking in some arbitrary computations in the case of missing data via the prepare_inputs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a reasonable compromise?
if self.dc_model == sdm_campanelli and 'F' in weather.columns and 'H' in weather.columns:
mc.F = weather['F']
mc.H = weather['H']
else:
self.pepare_inputs ... self.temp_model()
self.dc_model()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for using better variable names for F
and H
, I propose this in ModelChain.run
:
self.prepare_inputs(times, weather)
if not (hasattr(self, 'F') and hasattr(self, 'H')):
# Not using a PV reference device or the info is incomplete
# Use conventional (meterological) weather information
self.aoi_model()
self.spectral_model()
self.effective_irradiance_model()
self.temp_model()
self.dc_model()
self.ac_model()
self.losses_model()
with the logic moved to the beginning of ModelChain.prepare_inputs
:
if weather is not None:
self.weather = weather
if 'F' in weather:
# Use PV reference device weather information for F
self.F = weather['F']
else:
# Clear any stale value
if hasattr(self, 'F'):
del self.F
if 'H' in weather:
# Use PV reference device weather information for H
self.H = weather['H']
else:
# Clear any stale value
if hasattr(self, 'H'):
del self.H
I think the new simulation value added here outweighs the possibility that future models may have different flavors of "F" and "H" to contend with. As I mentioned, I will switch to more descriptive names for these at the ModelChain
level, but I would like to keep F
and H
(as well as succinct parameter names) in my low level pvsystem.sdm_campanelli
and pvsystem.calcparams_campanelli
, because they better correspond to the variables in the literature references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. At present the only model that uses the F
and H
inputs is Campanelli, but it is possible that F
is more generally useful in place of effective_irradiance
calculated from weather in, e.g., SAPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwhanse I'm pretty sure that F
(and H
) may be generalized to "all" the single diode models, the only difference being how they propagate into the "auxiliary equations". The derivation is analogous for double-diode models as well. One of my (longer term) goals for pv-fit.com is to fit calibration data to multiple models at once and provide the user with a model "ranking" based on a goodness of fit metric.
Would you be able to refer me to where in the literature the SAPM definition of "effective irradiance" is derived, esp. with respect to the conventional meteorological datasets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thunderfish24 here: http://prod.sandia.gov/techlib/access-control.cgi/2004/043535.pdf
It's poorly explained. But, in a nutshell, effective irradiance
is defined in the text as "the fraction of the total solar irradiance incident on the module to which the cells inside actually respond", i.e., define 'respond'. Ee
is calculated from weather by Eq. 1 (to get Isc) then Eq. 7 (ratio of calculated Isc to a reference value). You can do the algebra and get an expression for Ee
in terms of beam and diffuse on the array's surface. We often substitute measurements from an appropriate reference cell for the calculated Ee
value.
return mc | ||
|
||
|
||
def get_F_H_from_sapm(effective_irradiance, temps, alpha_sc, irrad_ref, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue with get_
prefix here as in comment on get_calcparams_campanelli
. Why did you put this function in modelchain.py
rather than pvsystem.py
? Seems to me that it's more general purpose than the modelchain
api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to keep it closer to where it's used. I've said this before, but I don't really see PVSystem
doing much of anything other than being the keeper of module and inverter configuration state. I don't mind moving this into PVSystem
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that it might make sense to move this function to the module pvsystem
, but not necessarily into the class pvsystem.PVSystem
. So, function name changes aside, we'd have pvsystem.get_F_H_from_sapm
. One could optionally implement pvsystem.PVSystem.get_F_H_from_sapm
that dispatches work to pvsystem.get_F_H_from_sapm
, but that may or may not be of value.
I'm updating my |
pvlib python pull request guidelines
Thank you for your contribution to pvlib python!
You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:
git diff upstream/master -u -- "*.py" | flake8 --diff
and/or landscape.io linting service.docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.
Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):