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

Add PVSPEC spectral correction factor model #2072

Merged
merged 79 commits into from
Jun 14, 2024
Merged

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented May 28, 2024

This model is one of the spectral correction functions suggested in the cited issues (#1950 and #2065).
Docs: https://pvlib-python--2072.org.readthedocs.build/en/2072/reference/generated/pvlib.spectrum.spectral_factor_pvspec.html

first attempt, PVSPEC model for the spectral mismatch factor based on air mass and clearness index
@echedey-ls
Copy link
Contributor

There are a few things that need to be done first:

  1. Separate the implementation into an existing or new file. E.g.: pvlib/spectrum/mismatch.py
    (That's moving the current function you have to there, modifying the needed __init__.py files too; I think your current test_pelland.py file is not a test, it seems like an implementation of the model)
  2. Create tests under pvlib/tests/test_spectrum.py. Prefix tests with test_ as @cwhanse noted, so they can be discovered automatically by pytest. I don't know exactly the pytest command to run exactly one test by name, but you could run that file by itself there. Or keep that file temporarily.

In general, you may want to see some PRs that add new functions as a reference. For example, #2041 is simple IMO. There are many more of course, it's just I'm too selfish to look for other ones (/sarcasm)

I've been inspired by #2067 to do a post blog regarding my setup. I know I wrote too much, but in case it helps you, check it out here.

@RDaxini RDaxini marked this pull request as draft May 30, 2024 13:10
add new spectral factor (to be tested)
delete new py file (and add new model into mismatch.py)
correct errors raised
create new tests for pelland air mass / clearness index spectral correction
Correct an expected value
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
@RDaxini
Copy link
Contributor Author

RDaxini commented May 31, 2024

@echedey-ls @cwhanse Thank you for the help. I think I have updated the files with the correct names now (and also put the functions in the right places)
The tests that I have added into test_spectrum.py so far have passed, but are there any additional tests I need?
Something else I am unsure about is documentation. I added in text for the docs into mismatch.py but when I try to view the build here it looks just like the pvlib homepage rather than the text I included in mismatch.py so I wanted to ask how/where do I write/set up the documentation correctly?

@echedey-ls
Copy link
Contributor

Well done @RDaxini , it looks good!!

how/where do I write/set up the documentation correctly

Yeah, documented functions aren't discovered automatically, you need to specify in a rST file the function. In general, these files are under docs\sphinx\source\reference. Specifically, for this model you would add to docs\sphinx\source\reference\effects_on_pv_system_output\spectrum.rst.

I will review in short some of the common typos we all make in the docstrings.

Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

Some general feedback. I don't want to overwhelm you, but I've left a fair amount of comments.

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/tests/test_spectrum.py Outdated Show resolved Hide resolved
RDaxini and others added 8 commits June 4, 2024 18:34
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
update eqn in docs
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
RDaxini and others added 5 commits June 11, 2024 16:20
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
tried typesetting the url differently, will fix line character length after
@echedey-ls
Copy link
Contributor

Don't worry @RDaxini , we've all been there.

Btw, can you double check the "sodapro" link is what you intended? http://www.sodapro.com/gl/web-services/atmosphere/linke-turbidity-factor-ozone-watervapor-and-angstroembeta

It doesn't work (404). Ah, and Sodapro looks like a sandblasting company?

@kandersolar
Copy link
Member

I believe https://www.soda-pro.com/ is the correct website ;)

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.

Editorial mostly, I think that matching output type to input type has slso been raised by others.

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved
pvlib/tests/test_spectrum.py Outdated Show resolved Hide resolved
RDaxini and others added 6 commits June 12, 2024 23:10
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
specific data resource via extended url is either behind a paywall or moved; linking the main site homepage instead
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@echedey-ls
Copy link
Contributor

@kandersolar @AdamRJensen or @cwhanse can you approve the workflows on this PR?

@kandersolar
Copy link
Member

@RDaxini one last thing is needed: a "what's new" entry for v0.11.0. Can you make a new bullet point under Enhancements in v0.11.0.rst, and list yourself in the Contributors list at the bottom of the file?

@kandersolar kandersolar added this to the 0.11.0 milestone Jun 13, 2024
@kandersolar kandersolar changed the title New spectral correction Add PVSPEC spectral correction factor model Jun 13, 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.

LGTM with one minor syntax edit. Thanks @RDaxini!

docs/sphinx/source/whatsnew/v0.11.0.rst Outdated Show resolved Hide resolved
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
@kandersolar kandersolar merged commit 19c9598 into pvlib:main Jun 14, 2024
25 checks passed
@RDaxini
Copy link
Contributor Author

RDaxini commented Jun 14, 2024

Thanks for all the reviews @echedey-ls @cwhanse @IoannisSifnaios @kandersolar

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.

None yet

5 participants