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

Agrivoltaics - PAR diffuse fraction model #2048

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented May 12, 2024

  • Closes Photosynthetically active radiation decomposition model #2047
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference 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 (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Adds the model described in #2047.

Current implementation and next steps

Added a new module called par. I still haven't added the index and API entries since I would like to have the green light before pursuing this path.

I'm a bit hesitant about the tests here. I haven't found any straightforward way to make the tests from the papers. They check the mathematical integrity of the implementation, if that's a thing.

Docs links

Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

Just an early stage review.

pvlib/par.py Outdated Show resolved Hide resolved
pvlib/par.py Outdated Show resolved Hide resolved
pvlib/par.py Outdated
Comment on lines 26 to 27
par_diffuse_fraction : numeric
Photosynthetically active radiation in W/m^2.
Copy link
Member

Choose a reason for hiding this comment

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

@kandersolar Do we have a standard format for specifying units? I've seen at least three different methods in our documentation:

  • ... active radiation in W/m^2.
  • ... active radiation. W/m^2.
  • ... active radiation. [W/m^2]

Copy link
Member

Choose a reason for hiding this comment

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

This question just came up elsewhere as well, with an additional contender of :math:`W/m^2`.

I don't think we have anything that rises to the level of a "standard", but I personally favor [W/m^2] unless there is good reason for something else. Reasons:

  • It makes sense to me for the unit to be visually separated from the text description (i.e. not in the same sentence) as it may prevent ambiguity when other quantities are mentioned in the description. So "... radiation in W/m^2" is out.
  • The :math: version results in bold font that looks a little jarring to me at the end of each line.
  • I think (but am not 100% confident) that [W/m^2] is the most common form currently in pvlib, so consistency supports keeping it.

If I am being honest, the main reason for me might just be that I am used to [W/m^2]. Anyway, that's my two (three?) cents!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the reasoning. There's still another alternative, which is using the ascii character superscript two:
[W/m²]
I personally prefer this one, to me it's like the caret adds visual noise.

pvlib/tests/test_par.py Outdated Show resolved Hide resolved
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
@AdamRJensen AdamRJensen added the GSoC Contributions related to Google Summer of Code. label May 24, 2024
@echedey-ls echedey-ls marked this pull request as ready for review May 24, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement GSoC Contributions related to Google Summer of Code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Photosynthetically active radiation decomposition model
3 participants