-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Quantum efficiency & spectral response conversion funcs #2041
Quantum efficiency & spectral response conversion funcs #2041
Conversation
…vity_to_quantum_efficiency
Since it messes the links in the toc table.
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 brief partial review :)
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Thanks @kandersolar , good points |
Perhaps you could give some thought to normalization. Although QE and SR can have absolute units, they are often produced and used as relative values. |
I didn't know about that! What kind of normalization is done, just each value over the maximum? I'm not sure if it makes much sense for a conversion function to do that. May it be misleading to do that here? |
I like the idea of optional |
Done! Let me know what you think about them. I've added a general-purpose function to do the normalization under |
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.
These look great! A few comments below, and there's a merge conflict to fix.
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com> Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
For me this one is also ready to go 🚀 ! Maybe do @RDaxini or @IoannisSifnaios want to review as well 👀 ? Mostly for reference of where the least amount of changes need to be done when creating new functions. |
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 looks good to me! I'll leave it open for a few days in case @IoannisSifnaios or @RDaxini want to review as well :)
Thanks @echedey-ls! |
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.Although I've opened an issue, I doubt there is much to discuss about these functions behaviour: possibly citations and error messages IMO.
Anyway, feel free to reach about any question, suggestion, etc.
Docs