-
Notifications
You must be signed in to change notification settings - Fork 998
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
Create function to calculate average photon energy #2140
Conversation
update docstring
fix docs typesetting
add checks in function for negative irradiance and invalid data type add tests for these checks update docs maths typesetting
still trying to typeset the units without using / / between multiple length units
trying unicode superscript
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
In general terms of the API, is it reasonable to add an optional parameter for the wavelength (pandas object index)? For example, polars users are understandably afraid of indexes https://docs.pola.rs/user-guide/migration/pandas/#polars-does-not-have-a-multi-indexindex . Other pandas alternatives that state performance improvements may also have indexes, so it may be worth to check against the My idea is to apply a pattern like this (edit: forget about the pvlib-python/pvlib/spectrum/response.py Line 184 in 72185cb
Nevertheless, I would understand opinions against this suggestion. The code may be a little more messy doing so. Otherwise, I think this PR is well packed. |
My 2 cents: the idea of making design decisions around non-pandas dataframes deserves more thorough discussion than what it will get in this PR. In this particular case, I think it should be easy to add that functionality later in a backwards-compatible way, so I think we should keep it simple and stick with the pandas use case for now. |
update returns statement, change symbols, suppress runtimewarning for division by zero Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe include one more test verifying "returns np.nan
in the case of 0 Wm⁻²nm⁻¹ spectral irradiance input."
add test for return np.nan in case of zero si input
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks @RDaxini
apple suggestions from code review Co-Authored-By: Adam R. Jensen <39184289+adamrjensen@users.noreply.github.com>
"spectrum" variable --> "spectra" (see pvlib#2150)
"spectrum" variable --> "spectra" (see pvlib#2150)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments :)
remove unintentional change
apply suggestion from code review Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
sentence correction Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
return series in case of dataframe input, add test to verify. Remove unnecessary spaces from tests
update test for datatype according to suggestion from code review
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Thanks @RDaxini and reviewers! |
Thanks all:) |
docs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.docs: https://pvlib-python--2140.org.readthedocs.build/en/2140/reference/generated/pvlib.spectrum.average_photon_energy.html