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

Quantum efficiency & spectral response conversion funcs #2041

Conversation

echedey-ls
Copy link
Contributor

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

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

@kandersolar kandersolar added this to the 0.11.0 milestone May 10, 2024
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.

A brief partial review :)

docs/sphinx/source/whatsnew/v0.10.5.rst Outdated Show resolved Hide resolved
docs/sphinx/source/whatsnew/v0.11.0.rst Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
echedey-ls and others added 3 commits May 10, 2024 10:57
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>
@echedey-ls
Copy link
Contributor Author

Thanks @kandersolar , good points

@adriesse
Copy link
Member

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.

@echedey-ls
Copy link
Contributor Author

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?

@kandersolar
Copy link
Member

I like the idea of optional normalize parameters that, when set to True, normalize the return value so that the maximum value is 1.0. I lean towards it defaulting to False.

@echedey-ls
Copy link
Contributor Author

Done! Let me know what you think about them. I've added a general-purpose function to do the normalization under pvlib.tools. New docs links (updated in PR body):

@AdamRJensen AdamRJensen added the GSoC Contributions related to Google Summer of Code. label May 24, 2024
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.

These look great! A few comments below, and there's a merge conflict to fix.

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/tests/test_spectrum.py Outdated Show resolved Hide resolved
pvlib/tests/test_spectrum.py Show resolved Hide resolved
pvlib/tests/test_tools.py Outdated Show resolved Hide resolved
echedey-ls and others added 3 commits May 24, 2024 22:28
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
echedey-ls and others added 2 commits May 25, 2024 01:32
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
@echedey-ls
Copy link
Contributor Author

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.

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.

This looks good to me! I'll leave it open for a few days in case @IoannisSifnaios or @RDaxini want to review as well :)

@kandersolar kandersolar merged commit 555b29a into pvlib:main Jun 10, 2024
30 checks passed
@kandersolar
Copy link
Member

Thanks @echedey-ls!

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.

Spectral responsivity and quantum efficiency conversion functions
4 participants