8000 Implement CEC model by cwhanse · Pull Request #560 · pvlib/pvlib-python · GitHub
[go: up one dir, main page]
Skip to content

Implement CEC model #560

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 13 commits into from
Sep 8, 2018
Merged

Implement CEC model #560

merged 13 commits into from
Sep 8, 2018

Conversation

cwhanse
Copy link
Member
@cwhanse cwhanse commented Sep 5, 2018 •
edited
Loading

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

  • Closes Implement CEC module performance model in place of desoto #463
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

@wholmgren
Copy link
Member

@cwhanse I suggest you wait to resolve the merge conflict until #559 is merged (as soon as tests pass). One way to do it will be:

  1. git fetch upstream
  2. git merge upstream/master -X theirs
  3. reapply and recommit your changes to DC_MODEL_PARAMS

There are more elegant ways to do it but this is probably the most straightforward.

@stickler-ci
Copy link
stickler-ci bot commented Sep 6, 2018

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

stickler-ci[bot]
stickler-ci bot reviewed Sep 6, 2018
View reviewed changes
wholmgren
wholmgren reviewed Sep 6, 2018
View reviewed changes
Copy link
Member
@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Looking good so far. Seems like we're only missing test_pvsystem.py tests and API/whatsnew documentation.

@wholmgren wholmgren added this to the 0.6.0 milestone Sep 7, 2018
stickler-ci[bot]
stickler-ci bot reviewed Sep 7, 2018
View reviewed changes
@cwhanse
Copy link
Member Author
cwhanse commented Sep 7, 2018

How do I resolve the remaining stickler issues? I'm not finding any open comments, and I've addressed the comments that I did find.

@wholmgren
Copy link
Member

@cwhanse I don't know. I have no problem merging pull requests that stickler hasn't passed. It's only a guide to help us.

wholmgren
wholmgren reviewed Sep 7, 2018
View reviewed changes
@cwhanse
Copy link
Member Author
cwhanse commented Sep 8, 2018

Ready to go, test failure is related to pulling data for the forecast unit tests.

@wholmgren
Copy link
Member

I still don't understand why we are maintaining compatibility with the old calcparams_desoto API in the new calcparams_cec. What is the sequence of events that would lead to an issue?

Within pvlib itself, the ModelChain path will go through a PVSystem.calcparams_cec/desoto method rather than calling the function directly, and we already updated those PVSystem methods to use the new function signature.

User code that directly uses calcparams_desoto will emit the warning and will need to be updated. User code that directly uses calcparams_desoto but should be updated to calcparams_cec can adapt to the new API at that time. Now that I think about it, perhaps the warning in calcparams_desoto should also tell people to consider using the calcparams_cec function.

@cwhanse
Copy link
Member Author
cwhanse commented Sep 8, 2018

I agree with your reason for removing it, I just forgot to do it.

@wholmgren
Copy link
Member

Great. Merging.

@wholmgren wholmgren merged commit 710e27e into pvlib:master Sep 8, 2018
@cwhanse cwhanse deleted the CECmodel branch September 21, 2018 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wholmgren wholmgren wholmgren left review comments

@stickler-ci stickler-ci[bot] stickler-ci[bot] left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None 4458 yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@cwhanse @wholmgren
0