Skip to content

Propagate parameters on calcparams_cec call (#1215)#1216

Merged
cwhanse merged 3 commits into
pvlib:masterfrom
Peque:fixup
May 27, 2021
Merged

Propagate parameters on calcparams_cec call (#1215)#1216
cwhanse merged 3 commits into
pvlib:masterfrom
Peque:fixup

Conversation

@Peque

@Peque Peque commented May 5, 2021

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

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

Peque commented May 5, 2021

Copy link
Copy Markdown
Contributor Author

@kanderso-nrel Thanks!

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

@cwhanse cwhanse 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.

Thanks @Peque

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

@wholmgren

wholmgren commented May 5, 2021

Copy link
Copy Markdown
Member

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

Peque commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

@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

Peque commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

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

@Peque

Peque commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

Should be fixed now. 😊

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

Thanks and good catch @Peque!

Comment thread docs/sphinx/source/whatsnew/v0.9.0.rst
Remove mistaken text
@wholmgren

Copy link
Copy Markdown
Member

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pvsystem.calcparams_cec() does not propagate parameters

4 participants