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

Bugfix: Unexpected behavior in SerialRefine caused by quiet NaNs #605

Merged
merged 6 commits into from Mar 17, 2023

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Mar 13, 2023

Description

If the farm power used in the Serial-Refine optimization method returns NaN (for example, some combination of yaw angles results in an invalid or unrealistic solution), then Serial Refine quietly tests the NaN case, finds it makes no improvement, and so defaults to no change. In the included example (75_check_sr.py), optimizing over 4 turbines (with a large range of yaw angles +/-50 deg to provoke the issue), before the suggested change to the argmax gives clearly suboptimal yaw angle result:

GCH optimal: [31.25 25. 0. 0. ]

In the proposed code change, by replacing argmax with nanargmax, we improve the situation by allowing the test comparison:

ids_better = (farm_powers_opt_new > farm_powers_opt_prev)

To use the best non-nan case, instead of using the nan case.

Additionally, to make this issue less silent, we issue a warning to the user.

With the change, the final output in example 75_check_sr.py is:

GCH optimal: [31.25 25. 18.75 0. ]

Impacted areas of the software

yaw_optimizer_sr.py

@paulf81 paulf81 added the bug Something isn't working label Mar 13, 2023
@paulf81 paulf81 self-assigned this Mar 13, 2023
@Bartdoekemeijer
Copy link
Collaborator

Hi @paulf81, interesting problem! Can you give an example why a farm power production value would be NaN? Also, maybe more a question to @rafmudaf, should we be raising warmings using the logger or is this fine?

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 14, 2023

Hi @Bartdoekemeijer, sorry! i missed all the comments I had originally included in the post I accidentally put into a comment box (@rafmudaf , maybe I can ask we can review the new format quickly? I couldn't tell where to put the body of the description of the pull request). To answer your question, if you try the temporary example 75 with nanargmin back to argmin you will see the issue. In this case we are using a too large yaw angle, but no the nan occurence is a little unpredictable. Then also, we see for large farms this can sometimes happen without very large angles, but haven't got it pinned yet.

@misi9170
Copy link
Collaborator

misi9170 commented Mar 15, 2023

@paulf81 thanks for doing this! @Bartdoekemeijer another use case is, as Paul mentioned, when there is a large FLORIS calculation going that could be producing NaNs for unknown reasons. When running within a serial refine operation, those NaNs would get silently ignored and serial refine would provide an output that is not optimal, which is confusing to the user. This happened to me recently because there were minor bugs in the new model (producing NaNs that I wasn't aware of). This new implementation will both

  • Let the user know that there were NaNs in the serial refine computation, and
  • Proceed to provide reasonable outputs even in the presence of NaNs.

Perhaps more often than not, the NaNs are coming from something buggy upstream, but I think it's still helpful for serial refine to call them out so that the user could go back and search for what causes them.

@rafmudaf
Copy link
Collaborator

Also, maybe more a question to @rafmudaf, should we be raising warmings using the logger or is this fine?

Yes, good catch @Bartdoekemeijer. The warning message should use the logging infrastructure. This allows to use a consistent formatting and integrates with the settings in the input file for logging location (stdout/stderr vs file) and logging level.

@Bartdoekemeijer
Copy link
Collaborator

Perhaps more often than not, the NaNs are coming from something buggy upstream, but I think it's still helpful for serial refine to call them out so that the user could go back and search for what causes them.

Ah that makes sense. I agree!

examples/75_check_sr.py Outdated Show resolved Hide resolved
@misi9170 misi9170 changed the title Bugfix serial refine if nans returned Bugfix: Unexpected behavior in SerialRefine caused by silent NaNs Mar 16, 2023
@misi9170 misi9170 changed the title Bugfix: Unexpected behavior in SerialRefine caused by silent NaNs Bugfix: Unexpected behavior in SerialRefine caused by quiet NaNs Mar 16, 2023
@rafmudaf
Copy link
Collaborator

@misi9170 @paulf81 do you intend for @Bartdoekemeijer and @bayc to review, as well?

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 17, 2023

I believe we're good to merge now @rafmudaf , thank you!

@rafmudaf rafmudaf merged commit 01684c8 into NREL:develop Mar 17, 2023
@paulf81 paulf81 deleted the bugfix/issue_in_sr branch March 17, 2023 16:17
paulf81 added a commit to paulf81/floris that referenced this pull request Jan 16, 2024
…L#605)

* Change argmax to nanargmax

* Add test example

* Add nan warning

* Updating warning message handling, removing temporary example.

* Cleaning up trailing whitespace as suggested by pre-commit.

* Remove unused import

---------

Co-authored-by: misha <msinner@nrel.gov>
Co-authored-by: Rafael M Mudafort <rafael.mudafort@nrel.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants