Skip to content
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

ADR PV efficiency model from pvpltools-python #1572

Closed
wants to merge 26 commits into from
Closed

Conversation

adriesse
Copy link
Member

@adriesse adriesse commented Oct 14, 2022

  • Partially fulfills Migrate efficiency models from pvpltools-python #1544
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@adriesse
Copy link
Member Author

I'm getting the ball rolling on this PR because there is some overlap with #1354 and it may be better to think ahead about how to best sort this out (assuming we want all these efficiency models in pvlib-python of course).

Both PR have a similar efficiency model function (mpm6() vs. mlfm_6() ) and both PR have a fitting function (fit_efficiency_model() vs. mlfm_fit() ). My opening suggestion would be to:

  • keep the efficiency model function with its peers in the new pvefficiency module
  • simplify mlfm_fit() by calling fit_efficiency_model() with the appropriate bounds as kwarg.

@steve-ransome
Copy link

steve-ransome commented Oct 14, 2022

I agree we should unify coding into sensible locations as soon as possible.

  1. mpm fits not just the performance ratio but also other normalised coefficients such as i_sc, r_sc, i_mp, i_ff ... v_oc, ff.
    Will this code only fit performance ratio or are other parameters allowed?

  2. The SAPM has 4 different equations to fit measured i_sc, i_mp, v_mp and v_oc, which can then be used to generate p_mp, pr_dc. Will this be added?

  3. The equation you used for PVGIS needs a correction factor for any degradation or eta_meas/eta_nameplate at STC

@adriesse
Copy link
Member Author

I agree we should unify coding into sensible locations as soon as possible.

  1. mpm fits not just the performance ratio but also other normalised coefficients such as i_sc, r_sc, i_mp, i_ff ... v_oc, ff.
    Will this code only fit performance ratio or are other parameters allowed?

Yes, if mpm has those capabilities, why not? Is that in the current code already, or something you're thinking about adding later?

  1. The SAPM has 4 different equations to fit measured i_sc, i_mp, v_mp and v_oc, which can then be used to generate p_mp, pr_dc. Will this be added?

That's not on my radar, but clearly both 1. and 2. could lead to different code organization ideas.

  1. The equation you used for PVGIS needs a correction factor for any degradation or eta_meas/eta_nameplate at STC

Yes, I wrote about this problem, but for me the model is primarily of historical interest.

@steve-ransome
Copy link

I agree we should unify coding into sensible locations as soon as possible.

  1. mpm fits not just the performance ratio but also other normalised coefficients such as i_sc, r_sc, i_mp, i_ff ... v_oc, ff.
    Will this code only fit performance ratio or are other parameters allowed?

Yes, if mpm has those capabilities, why not? Is that in the current code already, or something you're thinking about adding later?

It's already in the code. You make var_to_fit as 'pr_dc', 'v_mp', 'r_sc' or whatever and it will fit it. The jupyter notebooks only have the 'pr_dc' in there but I've fitted all the other parameters in my publications such as this http://www.steveransome.com/pubs/2206_PVSC_49_Ransome_to_be_presented.pdf

def mlfm_fit(data, var_to_fit):
var_to_fit : string
Column name in data containing variable being fitted.

  1. The SAPM has 4 different equations to fit measured i_sc, i_mp, v_mp and v_oc, which can then be used to generate p_mp, pr_dc. Will this be added?

That's not on my radar, but clearly both 1. and 2. could lead to different code organization ideas.

OK, as I knew SAPM and PVGIS are commonly used I analysed them in the paper I just linked. I also looked at the bilinear interpolation but as we've discussed before I really don't think it's a good idea at all. The traces are not linear with irradiance.

  1. The equation you used for PVGIS needs a correction factor for any degradation or eta_meas/eta_nameplate at STC

Yes, I wrote about this problem, but for me the model is primarily of historical interest.

OK, I have seen others acknowledge the problem and correct for it. I changed the 1 to be k_0 to solve that.

@adriesse
Copy link
Member Author

Well, maybe my opening suggestion was not the best in light of mpm/mlfm being used with a gradually expanding number of predictors and also applied to a multiple response variables. Perhaps it is better to see how #1354 evolves before trying to solve this.

@adriesse adriesse changed the title PV efficiency models from pvpltools-python ADR PV efficiency model from pvpltools-python Nov 23, 2022
@adriesse adriesse closed this Nov 27, 2022
@adriesse adriesse deleted the eta branch November 27, 2022 20:54
@adriesse
Copy link
Member Author

adriesse commented Nov 27, 2022

FYI: I messed up my branch so I started over in #1602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants