Skip to content

ADR PV efficiency model from pvpltools-python (take 2) #1602

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Dec 20, 2022
Merged

ADR PV efficiency model from pvpltools-python (take 2) #1602

merged 23 commits into from
Dec 20, 2022

Conversation

adriesse
Copy link
Member

@adriesse adriesse commented Nov 27, 2022 •
edited by kandersolar
Loading

  • 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

adriesse commented Nov 27, 2022 •
edited
Loading

FYI: I messed up my branch so I started over.

@adriesse adriesse mentioned this pull request Nov 27, 2022
Closed
8 tasks
@adriesse
Copy link
Member Author

Looking forward to some reviews...

@kandersolar kandersolar added this to the 0.9.4 milestone Nov 28, 2022
@adriesse
Copy link
Member Author

adriesse commented Dec 3, 2022

Now that the tests pass again I'm going to leave this alone for a few days so reviewers don't have to aim for a moving target. I'd be happy to receive general (non-code) comments on the gallery examples as well.
Link to gallery examples.

@adriesse
Copy link
Member Author

adriesse commented Dec 5, 2022

@kdebrab would you be interested in reviewing?

@mikofski mikofski self-requested a review December 7, 2022 17:38
@mikofski
Copy link
Member

mikofski commented Dec 7, 2022

Hi @adriesse thanks for this. I'll try to take a look at this asap.

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

@nicolasholland would you be interested in reviewing?

kandersolar
kandersolar reviewed Dec 12, 2022
View reviewed changes
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.

FYI gallery section ordering is configurable, but I suggest leaving that for the future. https://sphinx-gallery.github.io/stable/configuration.html#sorting-gallery-subsections

adriesse and others added 3 commits December 12, 2022 22:03
@adriesse
Merge branch 'pvlib:main' into eta2
fbb2603
@adriesse @kandersolar
Apply suggestions from code review
19cdcbf 
Kevin's bug observations and identifications.  :)

Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@adriesse
Improvements after first review.
a64c66a
kandersolar
kandersolar approved these changes Dec 15, 2022
View reviewed changes
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.

LGTM other than a what's new entry and a couple minor points below. @mikofski do you still want to review?

@adriesse
Copy link
Member Author

Thanks Kevin. I just left a few things open for other reviewer(s) to comment on/react to before finalizing.

cwhanse
cwhanse reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

We need to correct the pv_modeling.rst file to inspect the documentation pages.

@kandersolar kandersolar mentioned this pull request Dec 15, 2022
Merged
8 tasks
cwhanse
cwhanse reviewed Dec 16, 2022
View reviewed changes
@kandersolar kandersolar merged commit a49123b into pvlib:main Dec 20, 2022
@adriesse
Copy link
Member Author

adriesse commented Jan 7, 2023

I'm puzzled. The documentation preview showed links to the examples in the API section, but I don't see them in the current version... should I open an issue?

@adriesse adriesse deleted the eta2 branch January 7, 2023 21:50
@kandersolar
Copy link
Member

kandersolar commented Jan 7, 2023 •
edited
Loading

Looks like it's from an error I made in #1598. I will submit a PR shortly to fix it. Thanks for noticing, @adriesse!

@kandersolar kandersolar mentioned this pull request Jan 7, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwhanse cwhanse cwhanse approved these changes

@kandersolar kandersolar kandersolar approved these changes

@mikofski mikofski Awaiting requested review from mikofski

Assignees
No one assigned
Labels
documentation enhancement
Projects
None yet
Milestone
0.9.4
Development

Successfully merging this pull request may close these issues.
4 participants
@adriesse @mikofski @kandersolar @cwhanse