add mlfm #1354
add mlfm #1354
Conversation
|
This is the MLFM paper as submitted (without the header) |
There was a problem hiding this comment.
@steve-ransome Thanks for this submission.
Here is my review of your core algorithm, esp. the normalization and fitting. I have not yet run any examples or looked at the error stacking, plotting functions, or tests.
The main issue I see is potentially more careful handling of the fits in the special case where no windspeed info is provided. I think this should be investigated a bit further and would be happy to discuss.
| try: | ||
| import matplotlib.pyplot as plt # noqa: F401 | ||
| import matplotlib | ||
| matplotlib.use('agg') |
There was a problem hiding this comment.
Configure the agg backend first before importing anything else from matplotlib?
Given the import matplotlib, is import matplotlib.pyplot as plt even necessary here?
pvlib/mlfm.py
Outdated
| T_STC = 25.0 # STC temperature [C] temperature_ref | ||
| T_HTC = 75.0 # HTC temperature [C] | ||
| G_STC = 1000.0 # STC irradiance [W/m^2] | ||
| G_LIC = 0.2 # LIC irradiance [kW/m^2] |
There was a problem hiding this comment.
It's a bit unexpected to change the units between G_STC and G_LIC, even though it is documented.
Also, is G_LIC actually used?
There was a problem hiding this comment.
They should be the same units, one was changed without the other. G_LIC hasn't been used here but will be used in later code changes as the MLFM can create useful performance values at other conditions such as LIC, HTC, NOCT etc. for validation.
There was a problem hiding this comment.
For completeness and explicitness, you could include G_HTC, and T_LIC, as well as the G_LTC and T_LTC (and perhaps G_NOCT) conditions defined in IEC 61853-1.
There was a problem hiding this comment.
NAME value # comment unit PV_LIB name
T_STC = 25.0 # STC temperature [C] temperature_ref
G_STC = 1000.0 # STC irradiance [W/m^2]not all yet used below , added here for completeness
T_LIC = 25.0 # LIC temperature [C]
G_LIC = 200.0 # LIC irradiance [W/m^2]T_HTC = 75.0 # HTC temperature [C]
G_HTC = 1000.0 # HTC irradiance [W/m^2]T_PTC = 55.0 # HTC temperature [C]
G_PTC = 1000.0 # HTC irradiance [W/m^2]G_LTC = 500.0 # HTC irradiance [W/m^2]
T_LTC = 15.0 # LTC temperature [C]G_NOCT = 800 # NOCT irradiance [W/m^2]
T_NOCT = 45 # NOCT temperature [C]
There was a problem hiding this comment.
Note that some of these such as T_NOCT depend on the module type so I have added an example value of 45.
T_PTC is also a ballpark figure as it is taken at T_amb=20 and has to assume a noct.
T_ptc ~ 20 + 1000/800 * (Noct -20) ~ 50C
https://sinovoltaics.com/learning-center/quality/ptc-pv-usa-test-conditions/
pvlib/mlfm.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| data : DataFrame |
There was a problem hiding this comment.
Would it be too verbose to include typical values for var_to_fit in the docstring for that argument?
pvlib/mlfm.py
Outdated
| c5_zero = 'wind_speed' not in data.columns | ||
| # if wind_speed is not present, add it | ||
| if c5_zero: | ||
| data['wind_speed'] = 0. |
There was a problem hiding this comment.
Technically, c5 is unidentifiable if the data do not contain windspeed information. As such, the algorithm may not converge robustly and may report convergence problems when you nonetheless keep c5 in the model. (Your bounds on c5 may temper the convergence issues to some degree.)
Unfortunately, scipy.optimize.curve_fit does not allow you to specify equal upper and lower bounds for c5 in this case, which here is presumably zero by convention. I can suggest a couple ways to work around this, but first I suggest examining the convergence info from scipy.optimize.curve_fit in a case where there is no windspeed info. (Do this by passing full_output=True and examining the additional mesg and ierint outputs.)
There was a problem hiding this comment.
I made comparison curve_fits using datasets n05667_Y13_R1k6_fClear_041.csv and x19074001_iec61853_041.csv, both of which have no wind_speed info. For the comparison, I removed the c5 coefficient altogether from mlfm_6 (instead using a new mlfm_5 function for func). The remaining coefficients appear to be stable between the two approaches without convergence errors in either (although the convergence criterion may change from gtol to ftol). I did not check the results of of the fit beyond this. It appears that it is ok to use mlfm_6 in all cases, but I would still recommend checking for convergence.
pvlib/mlfm.py
Outdated
| coeff_err = list(perr) | ||
|
|
||
| # save fit and error to dataframe | ||
| pred = mlfm_6(data, coeff[0], coeff[1], coeff[2], coeff[3], coeff[4], |
There was a problem hiding this comment.
I think that you can pass full_output=True to scipy.optimize.curve_fit, then get the pred variable pre-calculated as infodict["fvec"] from the additional output infodict from scipy.optimize.curve_fit.
full_output=True also provides two more outputs: mesg and ierint, which you should inspect for convergence information, possibly warning the user and/or passing back to the caller.
https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.curve_fit.html
| @@ -150,6 +150,17 @@ def has_numba(): | |||
| requires_siphon = pytest.mark.skipif(not has_siphon, | |||
| reason='requires siphon') | |||
|
|
|||
| try: | |||
| import matplotlib.pyplot as plt # noqa: F401 | |||
There was a problem hiding this comment.
Why is this import made here, since import matplotlib is made next?
There was a problem hiding this comment.
I'm 99% sure that the import matplotlib.pyplot as plt is not needed.
|
Because there are several mflm-related tutorial notebooks and supporting files, we may want to add a general |
|
We already do that. , there are directories for reference data, meas_gtw measurements+weather, figs and outputs |
I may have not been clear. I was thinking in terms of how this structure might better grow and coexist with other multi-file tutorials.
|
|
@steve-ransome In the example notebooks, don't use Windows-specific path separators, which don't work on Mac/Linux. For example, instead of |
|
Thanks, have updated mlfm to use that |
|
@steve-ransome Would it be helpful at this point for me to review all the comments that I initiated and resolve/follow up with them? |
|
Thanks @markcampanelli that would be very useful. I think I've got as far as I can for the moment. |
|
@steve-ransome I walked through the comment history, and there seems to be some unresolved ones from all the reviewers (including me). Not sure if the maintainers are open to arranging a conference call to walk through all of these? Personally I have few outstanding small issues, whereas my main issue is that the coefficient-solver information is not propagated back to the user, including a potential convergence failure. In particular, I think the |
This could be a more general question about how we would like fitting functions in pvlib to behave. |
…into mlfm # Conflicts: # pvlib/mlfm.py
…into mlfm # Conflicts: # pvlib/mlfm.py
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).