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

Fix the turbine.power function unit test #755

Merged
merged 6 commits into from Dec 7, 2023

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Dec 6, 2023

Fix the power unit test

The unit test of the power() function in turbine.py, test_power() in turbine_unit_test.py contained several errors which were masked by the fact that the tolerance value passed assert_allclose() function was very large. This pull request fixes these issues. Specifically:

  • Previously, the cp_interp function was passed to power, yielding a cp value multiplied by air_density. However, this is not expected. get_turbine_powers() in floris_interface.py passes self.floris.farm.turbine_power_interps, which are in W. Correcting this yields a p, and a power_truth both in W
  • The "truth" calculation of p was missing the air density term
  • The new test compares the successfully with a very small, default tolerance.
  • Added to the unit test are checks that in high wind speeds the power is 5MW (since the test case is the reference 5MW turbine) and at near 0 wind speeds, power should be close to 0
  • Both the test_function, and the doc-string for power implied that rotor_effective_velocities should be 5D (including the last 2 dimensions for grid). However, since these are rotor-averaged quantities they should be only 3D, and this pull request fixes that in the test, and in the doc-string of power() (it was not necessary to change the code)
  • Finally, a block of commented out code in turbine_unit_test.py was deleted

Impacted areas of the software

turbine.py
turbine_unit_test.py

@paulf81 paulf81 added bug Something isn't working v3 Label to denote focus on v3 in-progress Work is actively in progress labels Dec 6, 2023
@paulf81 paulf81 requested a review from rafmudaf December 6, 2023 04:43
@paulf81 paulf81 self-assigned this Dec 6, 2023
@rafmudaf rafmudaf removed the in-progress Work is actively in progress label Dec 6, 2023
@rafmudaf rafmudaf added this to the v3.6 milestone Dec 6, 2023
@rafmudaf rafmudaf changed the title Fix the power() unit test Fix the turbine.power function unit test Dec 6, 2023
Copy link
Collaborator

@rafmudaf rafmudaf left a comment

Choose a reason for hiding this comment

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

Well done @paulf81! Here are a few comments to expand the tests and add a little more clarity. Overall, this is great, thanks for fixing this.

tests/turbine_unit_test.py Outdated Show resolved Hide resolved
tests/turbine_unit_test.py Show resolved Hide resolved
tests/turbine_unit_test.py Show resolved Hide resolved
tests/turbine_unit_test.py Outdated Show resolved Hide resolved
@paulf81
Copy link
Collaborator Author

paulf81 commented Dec 6, 2023

Improvements made in commit: "Add github revisions":

  1. Include multiple turbine test
  2. Include test with grid dimension
  3. Include comment that test turbine is NREL 5MW
  4. Lower tolerance for 0 power test to 1 W

@rafmudaf
Copy link
Collaborator

rafmudaf commented Dec 7, 2023

I changed the array comparisons here to assert np.allclose(..., ...) instead of np.testing.assert_allclose(..., ...) because the defaults for relative and absolute tolerance are different:

This means np.allclose tests for 5 significant digits until numbers very close to zero and then it checks for the absolute tolerance. On the other hand, np.testing.assert_allclose always tests for 7 significant digits. In this particular case, it's the rtol that fails in the power calculation, so switching comparison methods also relaxes the relative tolerance.

@rafmudaf rafmudaf merged commit b6de318 into NREL:develop Dec 7, 2023
8 checks passed
@misi9170 misi9170 mentioned this pull request Apr 5, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v3 Label to denote focus on v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants