Purdue-bifacial-irradiance-model

[nappaillav]

• added Purdue Bifractal irradince model pvlib/pvl_Purdue_bifacial_irradiance.py and pvlib/pvl_Purdue_albedo_model.py

• added trignometric functions in pvlib.tools.py

• tests are yet to be added

This is related to issue #863

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

[nappaillav]

@cwhanse Can you assign a review for this pull request. I'll be happy to merge all the changes asap :)

[wholmgren]

@nappaillav please review the contributing guidelines, in particular the sections on code style and testing. It will be much easier to review your code when it more closely conforms to pvlib style. For help with that, take a look at the diff in the files tab of this pr and make sure that it's very nearly clean of comments from the automated systems. Thanks in advance!

[cwhanse]

welcome back @nappaillav :) Glad to see this PR picked up again.

To answer your question: I'll be a reviewer, and I'd guess @mikofski will also be interested - he can nominate himself.

Before any reviews, could you:

• take care of all the stickler issues. Happy to help where it's not clear what formatting should be.
• use variable names common to the rest of pvlib

However, I have concerns about porting that Matlab code. In your absence, I started to work on #914 and ran into a number of questions about the code. In some places, the Matlab code does not follow the referenced paper, and there are steps in the Matlab code that aren't described in the paper. Before this PR could be merged we would have to resolve these issues, probably through communication with the original author.

[nappaillav]

@cwhanse Update the code with Stickler corrections
We can work on the necessary steps to port the matlab function into pvlib-python

[cwhanse]

@mikofski we need to come to agreement on how to structure pvlib.bifacial. Currently it contains one function, the wrapper for pvfactors. This PR should add to pvlib.bifacial but I'm not sure if that should be into pvlib.bifacial directly or as pvlib.bifacial.purdue or something similar. There's a non-trivial amount of code in this PR, and some pieces could be more general than the Purdue model.

The other contribution is in #717 which itself is a substantial body of functions. My guess is that some of those functions are also general in nature.

Ideally, a user could open pvlib.bifacial and find top-level functions for any bifacial model that pvlib supplies. But putting all the supporting functions for both in pvlib.bifacial would create a large code module. Maybe pvlib.bifacial.purdue and pvlib.bifacial.infinite_sheds is a practical first step.

[mikofski]

Ideally, a user could open pvlib.bifacial and find top-level functions for any bifacial model that pvlib supplies. But putting all the supporting functions for both in pvlib.bifacial would create a large code module. Maybe pvlib.bifacial.purdue and pvlib.bifacial.infinite_sheds is a practical first step.

How about making bifacial a subpackage, and then purdue and infinite_sheds could be submodules in the pvlib.bifiacial package. Is that what you're thinking? like:

+ pvlib
 \
  + bifacial
  |\
  | + __init__.py:  from pvlib.bifacial.pvfactors import pvfactors_timeseries  # keep api consistent
  | |
  | + purdue.py
  | |
  | + infinite_sheds.py
  | |
  | + pvfactors.py
  |
  + irradiance.py  # and other submodules & subpackages of pvlib

I can volunteer to review parts of purdue if it will help. Maybe we can split it up?

[cwhanse]

@mikofski yes, that's what I was imagining. There are other bifacial models in the wild which could migrate into or have a wrapper in pvlib.

@wholmgren @kanderso-nrel your views on this subject are needed.

[wholmgren]

I support a bifacial subpackage. I'm not sure about exposing the api through imports in __init__.py (aside from a deprecation period for the pvfactors location), but we can probably sort that out later.

[nappaillav]

@mikofski @cwhanse Can you please elaborate on next steps that I need to work on
And how should i write test for this functions

[cwhanse]

@nappaillav first step is to move the code into a new module pvlib.bifacial.purdue. Please change the function names to drop pvl_, and use only lower case in the names.

Next, I think we want to edit variable names to avoid upper case in most cases. python convention is that variables are all lower case, constants are all UPPERCASE, and Class definitions are Capitalized.

We need to also change variables to their pvlib name where this name exists. That's probably a step we can work through iteratively in reviews, but you can get started with it. I would guess that most variables that are not related to rear-surface irradiance or calculating view factors are already in pvlib somewhere.

[nappaillav]

@cwhanse There is already bifacial.py in the pvlib folder, hence to avoid confusion I added the bifacial model to bifacial_new. In future i'll will correct the folder name as per your suggestion and pull in the the pvlib/bifacial.py into pvlib/bifacial/

[cwhanse]

Closing due to merge of #717