Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Propagate parameters on calcparams_cec call (#1215) #1216

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
cwhanse merged 3 commits into from
May 27, 2021
Merged

Propagate parameters on calcparams_cec call (#1215) #1216

cwhanse merged 3 commits into from
May 27, 2021

Conversation

@Peque
Copy link
Contributor

@Peque Peque commented May 5, 2021 •
edited by cwhanse
Loading

  • Closes pvsystem.calcparams_cec() does not propagate parameters #1215
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

Not sure if tests are required (if so, which tests would be appropriate?). Not sure if an update to "whatsnew" is required.

@cwhanse cwhanse self-requested a review May 5, 2021 17:53
@cwhanse cwhanse added the bug label May 5, 2021
@cwhanse cwhanse added this to the 0.9.0 milestone May 5, 2021
@kandersolar
Copy link
Member

kandersolar commented May 5, 2021

For testing pass-through arguments we typically use the mocker fixture with assert_called_with like this: https://github.com/pvlib/pvlib-python/blob/master/pvlib/tests/test_modelchain.py#L1633-L1639

The failing tests are unrelated; looks like the PVGIS copyright text just changed again.

@Peque
Copy link
Contributor Author

Peque commented May 5, 2021

@kanderso-nrel Thanks!

I also added a comment in #776, since I think lint checks could help preventing these types of errors. 😊

cwhanse
cwhanse approved these changes May 5, 2021
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.

Thanks @Peque

Any changes to the CI tools (linting) should be proposed in a separate PR.

@wholmgren
Copy link
Member

wholmgren commented May 5, 2021 •
edited
Loading

New/better test would be great but optional since this is straightforward and I'm not too concerned about a regression. Definitely should note it in the whats new file.

@Peque
Copy link
Contributor Author

Peque commented May 6, 2021

@kanderso-nrel @wholmgren @cwhanse Added test and a short line in whatsnew.

Thanks for your feedback! 😊 👍

@Peque Peque force-pushed the fixup branch 2 times, most recently from 388dffa to 8a8a659 Compare May 6, 2021 10:03
@Peque
Copy link
Contributor Author

Peque commented May 6, 2021

I'll have a look at Python 3.6 and 3.7 failures!

@Peque
Copy link
Contributor Author

Peque commented May 6, 2021

Should be fixed now. 😊

@kandersolar kandersolar mentioned this pull request May 6, 2021
Closed
kandersolar
kandersolar approved these changes May 12, 2021
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.

Thanks and good catch @Peque!

Peque
Peque commented May 27, 2021
View reviewed changes
docs/sphinx/source/whatsnew/v0.9.0.rst Show resolved Hide resolved
@cwhanse
Update v0.9.0.rst
fa434f0 
Remove mistaken text
@wholmgren
Copy link
Member

wholmgren commented May 27, 2021

can this be merged?

@cwhanse cwhanse merged commit 5b0b15a into pvlib:master May 27, 2021
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

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

pvsystem.calcparams_cec() does not propagate parameters

4 participants

@Peque @kandersolar @wholmgren @cwhanse
Morty Proxy This is a proxified and sanitized view of the page, visit original site.