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

stream_temperature_function #2104

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

Conversation

IoannisSifnaios
Copy link
Contributor

@IoannisSifnaios IoannisSifnaios commented Jun 22, 2024

  • Related to Google Summer of Code: Floating PV #2068
  • 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.

Addition of the Stefan and Preud'homme equation for estimating the mean daily stream water temperature using the ambient air temperature as input. The paper can be found here.

Although this equation was created for estimating stream temperature, there are a number of publications that have used it for estimating the lakewater and seawater temperature (not sure if this is a good approach, though). From a quick search, some indicative publications that used it for estimating water temperature for floating PV are: 1, 2, 3, 4, 5.

Doc rendering

@IoannisSifnaios
Copy link
Contributor Author

IoannisSifnaios commented Jun 22, 2024

This function was not tested for very low mean daily ambient temperatures. Thus, if air temperatures lower than -6.6 degC are provided, the water temperature from this function will be negative. This can be true in some cases (e.g. if there is supercooling), but under normal conditions, it is not possible. So the question is: Should we limit the water surface temperature to 0 or return np.nan when low ambient air temperatures are used? I implemented the latter since I felt it was more in line with the paper.

@IoannisSifnaios IoannisSifnaios marked this pull request as ready for review June 22, 2024 11:08
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.

I can't access the paper so this is a bit of a blind review, but here are some initial ideas.

As always, feel free to make (or not) the changes with your own style.

pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/tests/test_floating.py Outdated Show resolved Hide resolved
pvlib/tests/test_floating.py Outdated Show resolved Hide resolved
@IoannisSifnaios
Copy link
Contributor Author

Thanks @echedey-ls and @AdamRJensen for all the help with the pytest.fixture.

Is there a way to use fixture values as input to a pytest.mark.parametrize test?

@cwhanse cwhanse added enhancement GSoC Contributions related to Google Summer of Code. labels Jun 24, 2024
@cwhanse cwhanse added this to the v0.11.1 milestone Jun 24, 2024
@cwhanse
Copy link
Member

cwhanse commented Jun 24, 2024

Looks good so far @IoannisSifnaios

pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
IoannisSifnaios and others added 6 commits June 24, 2024 22:43
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@kandersolar
Copy link
Member

Although this equation was created for estimating stream temperature, there are a number of publications that have used it for estimating the lakewater and seawater temperature (not sure if this is a good approach, though).

Perhaps it is worth noting this in the function's docstring?

pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Show resolved Hide resolved
pvlib/floating.py Show resolved Hide resolved
@adriesse
Copy link
Member

adriesse commented Jul 5, 2024

Thinking about this again, it seems unlikely that floating PV will be built on the type of water body modeled here, so are we encouraging the possible misuse for other types of bodies of water? I would suspect that a more suitable model for lakes and larger bodies exists somewhere.

IoannisSifnaios and others added 2 commits July 6, 2024 15:11
Co-authored-by: Anton Driesse <anton.driesse@pvperformancelabs.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
pvlib/floating.py Outdated Show resolved Hide resolved
pvlib/floating.py Outdated Show resolved Hide resolved
Co-authored-by: RDaxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Anton Driesse <anton.driesse@pvperformancelabs.com>
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.

I can't access the paper, but this proposal looks perfectly fine. Nice job!

Non-blocking comments down below.

Comment on lines +6 to +7
Functions
---------
Copy link
Contributor

Choose a reason for hiding this comment

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

I always wonder if a subsection like this is needed. I mean, it doesn't provide relevant info IMO.

An example in official docs:

Rendering in API page:
imagen

https://pvlib-python.readthedocs.io/en/stable/reference/tracking.html

Which in API index looks like:
imagen

I don't mean to request changes with respect to this for now, I will remove that official example if it renders correctly in a PR. Will be back in short, I'm having some problems at building.

pvlib/floating.py Outdated Show resolved Hide resolved
@IoannisSifnaios
Copy link
Contributor Author

IoannisSifnaios commented Jul 17, 2024

Thinking about this again, it seems unlikely that floating PV will be built on the type of water body modeled here, so are we encouraging the possible misuse for other types of bodies of water? I would suspect that a more suitable model for lakes and larger bodies exists somewhere.

@adriesse I also thought about this (and asked some oceanography researchers regarding models for sea temperature), but there are no simple models that can estimate the sea surface temperature, which I think is the reason why people used this model in the first place (although it might not be very appropriate). Thus, since we have mentioned the limitations of the model very explicitly, I still think it makes sense to add it in lack of a more appropriate model. Hopefully, in the future, some simple model might be developed, and then it could be directly compared to this one in order to estimate how "off" the results are...

IoannisSifnaios and others added 3 commits July 17, 2024 17:15
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@adriesse
Copy link
Member

Thinking about this again, it seems unlikely that floating PV will be built on the type of water body modeled here, so are we encouraging the possible misuse for other types of bodies of water? I would suspect that a more suitable model for lakes and larger bodies exists somewhere.

@adriesse I also thought about this (and asked some oceanography researchers regarding models for sea temperature), but there are no simple models that can estimate the sea surface temperature, which I think is the reason why people used this model in the first place (although it might not be very appropriate). Thus, since we have mentioned the limitations of the model very explicitly, I still think it makes sense to add it in lack of a more appropriate model. Hopefully, in the future, some simple model might be developed, and then it could be directly compared to this one in order to estimate how "off" the results are...

I haven't read these papers, but it sounds pretty questionable to take a model out of one context and use it in another context without some kind of checks or validation. If we put that in pvlib it's like giving someone a hammer when you know they need a screwdriver.

I suggest putting in an empty lake_temperature function that raises a NotImplementedError instead. Maybe that will encourage people to look into it!!

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

7 participants