add ASTM E1036 parameter extraction by mdeceglie · Pull Request #1585 · pvlib/pvlib-python · GitHub
Skip to content

add ASTM E1036 parameter extraction#1585

Merged
kandersolar merged 26 commits into
pvlib:mainfrom
mdeceglie:astm_e1036
Dec 14, 2022
Merged

add ASTM E1036 parameter extraction#1585
kandersolar merged 26 commits into
pvlib:mainfrom
mdeceglie:astm_e1036

Conversation

@mdeceglie

@mdeceglie mdeceglie commented Nov 2, 2022 •
edited by kandersolar
Loading

Copy link
Copy Markdown
Contributor
  • 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

mdeceglie commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

@cwhanse cwhanse mentioned this pull request Nov 8, 2022
Closed
kandersolar
kandersolar reviewed Nov 11, 2022
View reviewed changes

@kandersolar kandersolar left a comment

Copy link
Copy Markdown
Member

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.

Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
adriesse
adriesse reviewed Nov 11, 2022
View reviewed changes
Comment thread pvlib/ivtools/params.py Outdated
All rights reserved.
'''

df = pd.DataFrame()

@adriesse adriesse Nov 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@mdeceglie mdeceglie Dec 5, 2022

Copy link
Copy Markdown
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.

@mdeceglie @kandersolar
typo fix
a403a66 
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@kandersolar kandersolar added this to the 0.9.4 milestone Nov 21, 2022
@cwhanse

cwhanse commented Dec 5, 2022

Copy link
Copy Markdown
Member

@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
kandersolar
kandersolar approved these changes Dec 5, 2022
View reviewed changes

@kandersolar kandersolar left a comment

Copy link
Copy Markdown
Member

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?

Comment thread docs/sphinx/source/whatsnew/v0.9.4.rst Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
Comment thread pvlib/ivtools/params.py Outdated
mdeceglie and others added 3 commits December 6, 2022 08:45
@mdeceglie @kandersolar
fix docstring
1dface6 
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@mdeceglie @kandersolar
update reference
a817355 
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@mdeceglie @kandersolar
update whatsnew
f88e665 
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
mdeceglie and others added 2 commits December 6, 2022 08:47
@mdeceglie @kandersolar
update module description
2393b84 
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@mdeceglie
adjust first quadrant explanation
7294dfd
@mdeceglie

mdeceglie commented Dec 6, 2022

Copy link
Copy Markdown
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".

adriesse
adriesse reviewed Dec 7, 2022
View reviewed changes

@adriesse adriesse left a comment

Copy link
Copy Markdown
Member

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.

Comment thread pvlib/ivtools/utils.py Outdated
Comment thread pvlib/ivtools/utils.py Outdated
Comment thread pvlib/ivtools/utils.py Outdated
@kandersolar kandersolar mentioned this pull request Dec 9, 2022
Closed
9 tasks
@kandersolar

kandersolar commented Dec 14, 2022

Copy link
Copy Markdown
Member

@cwhanse OK to merge?

@cwhanse

cwhanse commented Dec 14, 2022

Copy link
Copy Markdown
Member

@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

Reviewers

@adriesse adriesse adriesse left review comments
@cwhanse cwhanse cwhanse approved these changes
@kandersolar kandersolar kandersolar approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

0.9.4

Development

Successfully merging this pull request may close these issues.

4 participants

@mdeceglie @cwhanse @kandersolar @adriesse