Skip to content

Change sapm to expect effective_irradiance in W/m2 #609

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.

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

Closed
wants to merge 11 commits into from

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Oct 19, 2018

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 Effective irradiance units #472
  • 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 LGTM and SticklerCI checks.
  • 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

This feels like a change for 0.7 instead of 0.6.1 because it will break code. I'm not opposed to moving forward with a 0.7 release and skipping a 0.6.1 release.

@cwhanse
Copy link
Member Author

cwhanse commented Oct 19, 2018

OK with me if we leave this for 0.7. It's an enhancement for consistency in the function level APIs, not a bug fix.

@cwhanse
Copy link
Member Author

cwhanse commented Oct 24, 2018

OK, leaving this PR without whatsnew changes for 0.7

@wholmgren wholmgren added this to the 0.7.0 milestone Nov 3, 2018
@wholmgren wholmgren added the api label Nov 3, 2018
@cwhanse cwhanse closed this Feb 4, 2019
@cwhanse cwhanse deleted the eff_irrad_units branch February 4, 2019 16:50
@wholmgren
Copy link
Member

@cwhanse do you no longer want to change the units? If so, we should close the corresponding issue. If not, perhaps we should merge this and make 0.7 the next release. I also have no objections to leaving it open during a 0.6.2 cycle.

@cwhanse
Copy link
Member Author

cwhanse commented Feb 5, 2019

I still want to change the units. I was doing some housecleaning in my fork and deleted this branch without realizing that it is connected to this PR.

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.

2 participants