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

feat(wind): Add power law interpolation method for wind conversion #402

Merged
merged 7 commits into from
Nov 11, 2024

Conversation

coroa
Copy link
Member

@coroa coroa commented Nov 1, 2024

Closes #231 .

Refer to issue for context. Implements the power law interpolation method as a new argument
to cutout.wind. Needs to be chosen manually for now.

A suggested in the issue we retrieve 100m and 10m wind speeds and then calculate a wind shear exponent from those during cutout preparation.

Looks like the power law is preferred at least for turbine heights above wind blending (?) height (70m?). So this should most likely become the new default if we have 100m wind speeds available (as we have for ERA5).

TODOs:

  • Compare results, maybe following methodology in https://www.mdpi.com/1996-1073/14/14/4169
  • Implement a FutureWarning that the default changes to the power law.
  • Add those interpolation options to the docs.

Changes proposed in this Pull Request

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • Newly introduced dependencies are added to environment.yaml, environment_docs.yaml and setup.py (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

@coroa
Copy link
Member Author

coroa commented Nov 1, 2024

Tests work locally, any pointers to what is going wrong in CI? @lkstrp

@coroa coroa requested a review from lkstrp November 1, 2024 20:41
@lkstrp
Copy link
Member

lkstrp commented Nov 3, 2024

This PR doesn't have permissions to get the CDS API secret because it comes from a fork, which causes these errors. We might want to change the trigger, or add a manual trigger that has permissions. But the Cutouts should also just be fully cached.

Copy link
Member

@lkstrp lkstrp left a comment

Choose a reason for hiding this comment

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

In pypsa and linopy we now use static types. So always feel free to spam those in there if you touch anything. Long way to go for atlite though.

And great to have you back 🎉

atlite/convert.py Outdated Show resolved Hide resolved
atlite/wind.py Outdated Show resolved Hide resolved
atlite/wind.py Outdated Show resolved Hide resolved
atlite/wind.py Outdated Show resolved Hide resolved
Copy link
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

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

I like it!

atlite/datasets/era5.py Show resolved Hide resolved
coroa and others added 2 commits November 9, 2024 18:29
Co-authored-by: Lukas Trippe <lkstrp@pm.me>
@coroa
Copy link
Member Author

coroa commented Nov 9, 2024

This PR doesn't have permissions to get the CDS API secret because it comes from a fork, which causes these errors. We might want to change the trigger, or add a manual trigger that has permissions. But the Cutouts should also just be fully cached.

Makes sense.

@coroa
Copy link
Member Author

coroa commented Nov 10, 2024

In pypsa and linopy we now use static types. So always feel free to spam those in there if you touch anything. Long way to go for atlite though.

Added quite some typing to the wind conversion now. Needs typing-extensions and from __future__ import annotations though.

Hope that is agreed practice.

@lkstrp
Copy link
Member

lkstrp commented Nov 11, 2024

Added quite some typing to the wind conversion now. Needs typing-extensions and from __future__ import annotations though.

Hope that is agreed practice.

Great, it is. The __future__ import is only needed for Python 3.9, and we can remove it once we deprecate 3.9 (and support 3.13 instead).

…lanation

If auxiliary data is missing from the cutout.
@coroa
Copy link
Member Author

coroa commented Nov 11, 2024

Added quite some typing to the wind conversion now. Needs typing-extensions and from __future__ import annotations though.
Hope that is agreed practice.

Great, it is. The __future__ import is only needed for Python 3.9, and we can remove it once we deprecate 3.9 (and support 3.13 instead).

Yeah, well, i am confused about the state of the annotations :), since I started reading into https://docs.python.org/3.11/whatsnew/3.11.html#pep-563-may-not-be-the-future .

Copy link
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

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

LGTM.

Side note: Would it make sense to at some point split the wind_test into multiple smaller tests?

@coroa coroa merged commit 2e445cf into PyPSA:master Nov 11, 2024
3 of 4 checks passed
@coroa coroa deleted the wind-add-power-law-interpolation branch November 11, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use power low instead of log law for extrapolating wind speed
3 participants