Skip to content

add ASTM E1036 parameter extraction #1585

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

Merged
merged 26 commits into from
Dec 14, 2022
Merged

Conversation

mdeceglie
Copy link
Contributor

@mdeceglie mdeceglie commented Nov 2, 2022

  • 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.

This PR adds the ASTM E1036 methods for extracting performance parameters from IV curves. The code has been adapted from https://github.com/NREL/iv_params

@mdeceglie mdeceglie marked this pull request as draft November 2, 2022 19:51
@mdeceglie
Copy link
Contributor Author

Here is a start at adding ASTM E1036 IV curve parameter extraction. Open to any and all feedback including naming of function/module/parameters. The original code was object-oriented and included plotting functionality. I have simplified it to a stand alone function to avoid opening a whole can of worms about plotting and to align with the overall low-level functional building block approach of pvlib, but we can re-evaluate these choices if there is interest.

@cwhanse cwhanse mentioned this pull request Nov 8, 2022
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree plotting should not be in pvlib itself, at least at this time. Could be a good fit for the example gallery. I'm neutral on the pvlib.ivtools.params name, don't love it but can't think of anything better.

All rights reserved.
'''

df = pd.DataFrame()
Copy link
Member

@adriesse adriesse Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything could all be calculated with numpy only without many changes, if that is of interest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting suggestion. I don't think I will have time to tackle it, but if you feel it makes a sufficient improvement please go for it. Tests are in place now so it should be easy to verify that it has worked.

Co-authored-by: Kevin Anderson <[email protected]>
@kandersolar kandersolar added this to the 0.9.4 milestone Nov 21, 2022
@cwhanse
Copy link
Member

cwhanse commented Dec 5, 2022

@mdeceglie fyi we are still working out language for the copyright renewal. Thanks for your patience.

@mdeceglie mdeceglie marked this pull request as ready for review December 5, 2022 22:29
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few sphinx nitpicks but otherwise LGTM pending the copyright question.

I'm okay with the new pvlib.ivtools.params module name but I'm not really familiar with this area. Can we imagine any other related functionality we may want to add in the future that might make us wish we had chosen a different module name?

mdeceglie and others added 3 commits December 6, 2022 08:45
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
@mdeceglie
Copy link
Contributor Author

It might make sense to move this into the utils module which is described as containing "utility functions related to
working with IV curves, or fitting equations to IV curve data." I think this qualifies as "working with IV curves".

Copy link
Member

@adriesse adriesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments, not a complete review.

@kandersolar kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@kandersolar
Copy link
Member

@cwhanse OK to merge?

@cwhanse
Copy link
Member

cwhanse commented Dec 14, 2022

@cwhanse OK to merge?

Yes after the whatsnew conflict is resolved of course.

@kandersolar kandersolar merged commit a5f3ef4 into pvlib:main Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants