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

gallery example for bifacial modeling with pvfactors #1394

Merged
merged 14 commits into from
Feb 28, 2022

Conversation

spaneja
Copy link
Contributor

@spaneja spaneja commented Jan 26, 2022

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

The gallery does not currently have any examples for bifacial modeling. After looking into the issue, it appears that it is somewhat clunky to perform bifacial modeling with the modelchain. As such, I have created two examples of bifacial modeling, with the pvfactors approach, and would like the community to weigh in. One example uses the modelchain, while another is procedural. I will wait for feedback on which approach is more suited for the gallery.

At this time, much of the documentation inside the examples is not complete or is copy/pasted from another example. I will clean and polish this when a decision is made on modeling approach.

@kandersolar
Copy link
Member

Thanks @spaneja! I agree it makes sense to decide what we want the examples to do before getting into the nitty gritty about code formatting and such.

First though, the docs build is failing because ReadTheDocs doesn't currently install pvfactors before trying to run the examples. Can you update this PR and add a 'pvfactors' entry to this list in setup.py? https://github.com/pvlib/pvlib-python/blob/master/setup.py#L59-L62

added pvfactors into setup.py, cleaned up some white spacing, and hoping for the best...
@cwhanse
Copy link
Member

cwhanse commented Jan 27, 2022

Thanks @spaneja. I would vote to move forward.

Generally, I think the ModelChain approach is about as clean as pvlib v0.9.0 can support. This is an important example to add, which may also help us see how to deliver bifacial capabilities through ModelChain methods.

The procedural approach looks OK to me also, with the exception of the list structure for irrad; maybe that's a pvfactors issue but references like irrad[0] are rather opaque.

Ping me when you want to start working on commenting, etc.

@spaneja spaneja marked this pull request as ready for review January 27, 2022 21:22
@spaneja
Copy link
Contributor Author

spaneja commented Jan 27, 2022

Thanks @cwhanse and @kanderso-nrel. I have gone ahead and cleaned up and edited the submissions here. Please take a look for your approval and let me know of any feedback that you have.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @spaneja, here are some notes.

docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved

# create modelchain object for bifacial system and run bifacial simulation
mc_bifi = modelchain.ModelChain(system, site_location, aoi_model='no_loss')
mc_bifi.run_model_from_effective_irradiance(irrad)
Copy link
Member

Choose a reason for hiding this comment

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

It's a small inelegance that specifying aoi_model is necessary for a ModelChain that only uses run_model_from_effective_irradiance. I don't see a way around it, so I don't think there's anything to be done in this PR, just thinking out loud about the ModelChain design.

Copy link
Member

Choose a reason for hiding this comment

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

Could default to None but then we'd need to check for a valid model in other run_model methods.

Copy link
Member

Choose a reason for hiding this comment

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

Converted to #1411

@kandersolar
Copy link
Member

Referring to #1398, we should think about the choice of n_pvrows in these two examples. I think it would be a good idea to explicitly specify n_pvrows=something (so that at least the choice is explicit in the code instead of implicit via the default value) and leave a comment briefly explaining why the user may want to choose a different value.

docs/examples/bifacial/plot_bifi_model_mc.py Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved
docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
@campanelli-sunpower
Copy link

PSA: I have a "pre-release" of pvfactors v1.5.2 up at https://github.com/SunPower/pvfactors/releases/tag/v1.5.2. All that this left is to name it and for me to get cred's to upload to PyPI, see SunPower/pvfactors#137.

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.

Thanks @spaneja

docs/examples/bifacial/plot_bifi_model_mc.py Outdated Show resolved Hide resolved

# create modelchain object for bifacial system and run bifacial simulation
mc_bifi = modelchain.ModelChain(system, site_location, aoi_model='no_loss')
mc_bifi.run_model_from_effective_irradiance(irrad)
Copy link
Member

Choose a reason for hiding this comment

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

Converted to #1411

docs/examples/bifacial/plot_bifi_model_pvwatts.py Outdated Show resolved Hide resolved
docs/sphinx/source/whatsnew/v0.9.1.rst Outdated Show resolved Hide resolved
@kandersolar kandersolar merged commit f1c16f2 into pvlib:master Feb 28, 2022
@kandersolar
Copy link
Member

Thanks @spaneja!

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