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

Function to estimate wind speed at different heights #2124

Merged
merged 26 commits into from
Aug 5, 2024

Conversation

IoannisSifnaios
Copy link
Contributor

@IoannisSifnaios IoannisSifnaios commented Jul 10, 2024

  • Closes Expression for calculating wind speed at different heights #2118
  • 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.

The wind speed at different heights is calculated using the Hellmann power law expression (source 1, source 2). The Sandia method (discussed in #2118) is a subcase of this model, so it was added in the notes as a possible coefficient combination that could be used by the user.

@IoannisSifnaios IoannisSifnaios marked this pull request as ready for review July 10, 2024 16:37
Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

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

Nice work @IoannisSifnaios. Just had a quick read and picked up on a few small things for now

pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
@cwhanse
Copy link
Member

cwhanse commented Jul 10, 2024

Reactions rather than a review. Questions for anyone looking at this PR:

  1. Naming. Do we need a wind (prefer that to windspeed) code module? For now, the only use is in temperature functions so this new function could be placed in pvlib.temperature. Are there other wind-related functions that may be relevant? I don't think pvlib is the place to put something like a flow simulator to estimate dynamic forces on trackers, for example.

  2. Function name. Should be shorter. Does not need to repeat the input to output quantities. So may be as short as pvlib.wind.hellmann? That may be too terse.

@RDaxini
Copy link
Contributor

RDaxini commented Jul 11, 2024

I am not knowledgeable enough on the wind/PV subject to comment in detail on 1., but just my two cents on 2. at least...

  1. Function name. Should be shorter. Does not need to repeat the input to output quantities. So may be as short as pvlib.wind.hellmann? That may be too terse.

I am +1 for shortening the function name.
I think pvlib.wind.hellmann works fine, but pvlib.wind.windspeed_hellman might be clearer, especially if there are a range (not all wind speed) wind-related functions in pvlib.wind. That'd be similar to some other function formats, e.g. pvlib.spectrum.spectral_factor_sapm --> general subject, then output_model as the function name.

Or if all models in the module are going to be for calculating wind speed, then going back to a windspeed module name with a pvlib.windspeed.hellmann function seems okay. I'd be neutral on pvlib.wind.hellmann in that case.

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.

Nice approach, and sorry for the big points I've written down below. W.r.t. the model, does the height of interest need to be lower than the reference height? At least on wikipedia:

imagen

Btw, a point that may also interest @RDaxini, maybe other maintainers, is reviewing the docs log. It shows warning's, error's and critical's if there are any (well, there's always at least two warnings regarding setuptools replacing distutils and 2 I added unintentionally in a recent pr😞 ).

For example:

  1. go to this PR page or the official released one.
  2. in the version picker, select builds
  3. Find the last docs built for PR number, e.g. 2124
  4. Expand the last log, the one with python -m sphinx -T -b html ...
  5. Go find all the warnings, errors or criticals. Hint: use Ctrl+F, only works with expanded log.

pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/windspeed.py Outdated Show resolved Hide resolved
pvlib/tests/test_windspeed.py Outdated Show resolved Hide resolved
pvlib/tests/test_windspeed.py Outdated Show resolved Hide resolved
@AdamRJensen AdamRJensen mentioned this pull request Jul 13, 2024
@echedey-ls echedey-ls mentioned this pull request Jul 18, 2024
6 tasks
Co-authored-by: RDaxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@IoannisSifnaios IoannisSifnaios marked this pull request as draft July 19, 2024 12:01
IoannisSifnaios and others added 2 commits July 19, 2024 15:29
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@IoannisSifnaios
Copy link
Contributor Author

IoannisSifnaios commented Jul 19, 2024

Nice approach, and sorry for the big points I've written down below. W.r.t. the model, does the height of interest need to be lower than the reference height? At least on wikipedia:

imagen

@echedey-ls it is correct that Wikipedia mentions that constraint with the height; however, I can't find where this constraint has come from... the formula is reported in other places without the constraint (which I guess also makes sense since the expression still gives reasonable values...). Any other thoughts on this?

@IoannisSifnaios IoannisSifnaios marked this pull request as ready for review July 22, 2024 18:05
@IoannisSifnaios
Copy link
Contributor Author

I don't really get this error:
image

I am testing for a negative height (that would return a complex wind speed), which eventually returns a NaN value. Am I misunderstanding something?

@cwhanse
Copy link
Member

cwhanse commented Jul 22, 2024

isinstance is checking the type of wind_speed, which is either Series or ndarray. Its not checking the type of elements of wind_speed.

Rather than checking for and replacing complex values, I think I would use np.where on height_desired

@AdamRJensen
Copy link
Member

@echedey-ls it is correct that Wikipedia mentions that constraint with the height; however, I can't find where this constraint has come from... the formula is reported in other places without the constraint (which I guess also makes sense since the expression still gives reasonable values...). Any other thoughts on this?

Given that the reference we use doesn't state any constraint I think we shouldn't either if we can't find a solid reference for that. Also, the equation is reversible, so it produces consistent results estimating wind speeds at higher and lower height.

Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

I think we can remove all mentions of units as the equation only deals with ratios.

pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Outdated Show resolved Hide resolved
pvlib/atmosphere.py Show resolved Hide resolved
IoannisSifnaios and others added 3 commits August 5, 2024 17:19
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
docs/sphinx/source/whatsnew/v0.11.1.rst Outdated Show resolved Hide resolved
@IoannisSifnaios IoannisSifnaios marked this pull request as ready for review August 5, 2024 16:29
@AdamRJensen AdamRJensen dismissed adriesse’s stale review August 5, 2024 16:30

Review has been addressed

@AdamRJensen AdamRJensen merged commit f6b1d2a into pvlib:main Aug 5, 2024
30 checks passed
@AdamRJensen
Copy link
Member

I've merged the function as the reviewer comments have been addressed (a note was added about Albuquerque location and the function was renamed).

Thanks @IoannisSifnaios 🥳

@kandersolar kandersolar added this to the v0.11.1 milestone Aug 5, 2024
@kandersolar kandersolar added the GSoC Contributions related to Google Summer of Code. label Aug 5, 2024
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.

Expression for calculating wind speed at different heights
8 participants