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

Quantify wind speed shift #35

Closed
wants to merge 1 commit into from
Closed

Conversation

samuelwnaylor
Copy link
Collaborator

In similar way that power curve, rpm and pitch shifts are already calculated, the change in this PR adds wind speed shift to the calculated ops curve shifts.

A refactor on the ops curve shift functionality was performed as part of this PR to improve readability and ease of adding further shift calculations if required in future.

Copy link
Contributor

@aclerc aclerc left a comment

Choose a reason for hiding this comment

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

At a high level this looks very good. I like the new ops_curve_shift.py module and appreciate the thorough testing of it. Could you please re-run the smarteole notebook as part of this PR? This way the PR will include a real-world example of the new wind speed drift calculation for sanity checking.

@samuelwnaylor
Copy link
Collaborator Author

samuelwnaylor commented Oct 9, 2024

At a high level this looks very good. I like the new ops_curve_shift.py module and appreciate the thorough testing of it. Could you please re-run the smarteole notebook as part of this PR? This way the PR will include a real-world example of the new wind speed drift calculation for sanity checking.

Addressed in commit 6ff04f8

@samuelwnaylor samuelwnaylor force-pushed the quantify-wind-speed-shift branch 2 times, most recently from af5f8c6 to 6ff04f8 Compare October 9, 2024 11:26
@samuelwnaylor samuelwnaylor force-pushed the quantify-wind-speed-shift branch 15 times, most recently from 4263fe0 to 05b0c19 Compare November 14, 2024 20:40
@samuelwnaylor
Copy link
Collaborator Author

samuelwnaylor commented Nov 14, 2024

I've added the running and maintenance of smarteole_example.ipynb to GitHub Actions so that it is no longer a requirement to re-run the notebook manually before PRs are approved and merging into the main branch. In summary the change implements:

  • The .ipynb in the repo is stored without any cell execution or outputs (to ensure this is maintained in this state, the cell outputs are checked for emptiness as part of the CI, see the command poe smarteole-nb-clear-output --check)
  • The GitHub Actions executes the notebook and stores as html as an artifact. This artifact is attached to the action run and may be downloaded for review.

[UPDATE] The Smarteole example notebook is executed in the smarteole_example.yaml workflow on merge to main. The workflow may be triggered manually (workflow_dispatch:), which could be used if the PR review would like to see the notebook output before merge to main.

@samuelwnaylor samuelwnaylor force-pushed the quantify-wind-speed-shift branch 11 times, most recently from ae6fce2 to 59ae437 Compare November 15, 2024 17:34
@samuelwnaylor samuelwnaylor marked this pull request as draft November 15, 2024 17:34
@samuelwnaylor samuelwnaylor force-pushed the quantify-wind-speed-shift branch 2 times, most recently from 699ce81 to a046562 Compare November 15, 2024 18:06
@samuelwnaylor samuelwnaylor marked this pull request as ready for review November 15, 2024 18:19
@samuelwnaylor samuelwnaylor force-pushed the quantify-wind-speed-shift branch 4 times, most recently from c085f92 to e9ae4ae Compare November 18, 2024 21:16
smarteole_example.ipynb updated by GitHubAction
@github-actions github-actions bot closed this Nov 18, 2024
@samuelwnaylor samuelwnaylor removed the request for review from aclerc November 19, 2024 10:05
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.

2 participants