Skip to content
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

port Purdue bifacial irradiance model #914

Closed
wants to merge 7 commits into from
Closed

port Purdue bifacial irradiance model #914

wants to merge 7 commits into from

Conversation

nappaillav
Copy link
Contributor

@nappaillav nappaillav commented Feb 25, 2020

  • Closes #xxxx
  • 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.

This is related to issue #863

@cwhanse cwhanse added this to the 0.7.2 milestone Feb 25, 2020
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.

Test failure is for unrelated remote data

@wholmgren
Copy link
Member

@nappaillav are you planning on adding the bifacial code to this PR or did you want to split it into two PRs? My preference is a single PR since the trig functions are not explicitly tested and I don't see another near-term use for them.

@nappaillav
Copy link
Contributor Author

@wholmgren Initially I thought of having separate pull requests, but I guess its better to combine both as you suggest.

@CameronTStark CameronTStark modified the milestones: 0.7.2, 0.7.3 Mar 1, 2020
@nappaillav
Copy link
Contributor Author

There are few more steps in the process of completed the port of Purdue bifacial irradiance model, which are as follows

  1. Description for the modules/ functions
  2. Testing of the individual functions
  3. Coding Style has to be corrected

Currently I'm updating the code for your perusal. So that you can suggest me

@nappaillav nappaillav changed the title additional trig functions port Purdue bifacial irradiance model Mar 11, 2020
@nappaillav
Copy link
Contributor Author

@cwhanse I have updated the description. If you can let me know if the implementation is fine by you, we can proceed further.
I also need help from you to completing the unit test.
I myself will do the write the test_codes, if you can provide me the test data (Input and output) it would be great.

@cwhanse cwhanse modified the milestones: 0.7.3, 0.8.0 Mar 17, 2020
@cwhanse
Copy link
Member

cwhanse commented Mar 17, 2020

@cwhanse I have updated the description. If you can let me know if the implementation is fine by you, we can proceed further.

The new bifacial functions should be added to pvlib.bifacial.py.

I'd suggest
pvlib.bifacial.irradiance_purdue
pvlib.bifacial.ground_reflected_purdue

Although I'm not fond of the ground_reflected name, it will move us ahead.

I also need help from you to completing the unit test.

Sure. Before we do that, we need to clean up the commit - it's not reviewable in current form.

There's a lot of leftover Matlab formalities that need to be dropped or formatted to pass Stickler.

There's a bunch of recent commits to pvlib master in other PRs that show up here as new code changes @wholmgren should this be addressed with a git rebase?

@wholmgren
Copy link
Member

Yes

@nappaillav
Copy link
Contributor Author

I will rebase and make sure the commit is clean

@cwhanse cwhanse removed this from the 0.8.0 milestone May 19, 2020
@cwhanse
Copy link
Member

cwhanse commented Feb 23, 2022

Closing due to merge of #717

@cwhanse cwhanse closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants