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] Prevent unnecessary warning being raised in yaw optimization procedures #1045

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Dec 6, 2024

FLORIS v4.2.1 included a fix/enhancement #1031 for the yaw optimization routine. As part of that fix, I changed self.fmodel_subset = self.fmodel.copy() to a self.fmodel_subset = copy.deepcopy(self.fmodel) at this line of yaw_optimization_base.py. I did this to ensure that control setpoints were carried over to the fmodel_subset attribute. While this worked, it also copied over the wind_data stored at self.fmodel._wind_data.

In itself, that is not really a problem. However, in the yaw optimization routine, the wind_directions, wind_speeds, and turbulence_intensities are set() on self.fmodel_subset, see here. This overrides the stored _wind_data, which again is not an issue, except that it raises a warning because of these lines on the FlorisModel class. The repeated printout of this warning each time YawOptimization._calculate_farm_power() is called is tedious and disconcerting. This is pointed out by @ @MiguelMarante in #1016

To see this, one may run example 001_opt_yaw_single_ws.py in examples/examples_control_optimization/, and see the following printout
image
where previously, on v4.2 before #1031 was enacted, the printout was
image
(Note that the warning raised is once after the yaw optimization procedure is complete, but this is due to this line in the example and is the kind of situation the warning was intended for.)

To resolve this, I see a range of possible fixes:

  1. (currently implemented on this branch): Simply set self.fmodel_subset._wind_data = None after the self.fmodel_subset = copy.deepcopy(self.fmodel) statement on YawOptimization. This works well, because it sets the unused ._wind_data to None and the warning is then not raised. However, it accesses a private attribute, and is therefore pretty clearly a "hacky" solution.

  2. Create a method on FlorisModel, perhaps called reset_wind_data(), that sets the private attribute to None, and then call this method after the self.fmodel_subset = copy.deepcopy(self.fmodel) statement on YawOptimization. However, it should be noted that although this seems similar to reset_operation(), all it would do is override the _wind_data attribute with None. This is arguably a bit dangerous, because the wind_speeds, wind_directions, and turbulence_intensities that were set by the originally-passed WindData object would not be dropped (as intended), so the name and purpose of the method isn't very clear.

  3. Simply remove the warning raised in FlorisModel. I'm not sure how useful this warning is in any circumstance; however, there may be some unintended consequences in situations for which the warning was originally intended.

  4. A more thorough fix would be to allow wind_data, along with other arguments to FlorisModel.set(), to be explicitly removed. This is discussed in Allow "unsetting" non-critical keyword arguments to FlorisModel.set() #974.

@rafmudaf , @paulf81 , I'd like to hear your input on this. We actually originally did not store the wind_data on the FlorisModel at all, which would also have avoided this issue, but it was added in #849 (along with the warning) to make post-processing methods more straightforward. Looking back at #849, perhaps option (3.) above is not so bad; perhaps I was a little too liberal with my raising of warnings.

@misi9170
Copy link
Collaborator Author

misi9170 commented Dec 6, 2024

Rebasing now to be from main for this bugfix. Done.

@misi9170 misi9170 force-pushed the bugfix/yaw-opt-warning branch from 11bf579 to 5eceef4 Compare December 6, 2024 16:15
@misi9170 misi9170 marked this pull request as draft December 6, 2024 16:17
@misi9170 misi9170 marked this pull request as ready for review December 10, 2024 18:06
@misi9170
Copy link
Collaborator Author

misi9170 commented Dec 10, 2024

After discussing with @rafmudaf , we've decided that the currently-implemented bugfix (1. in the list above) is a reasonable solution. If we find that we are accessing the private _wind_data attribute often, we will look into a more rigorous fix.

This PR is designed for direct merger (via "squash and merge") into the main branch, after which I'll update the version number and create a new patch release.

@misi9170 misi9170 requested a review from rafmudaf December 10, 2024 18:07
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.

This looks reasonable to me.
Also, it's worth noting that in Python nothing is really private. So, renaming FlorisModel._wind_data to FlorisModel.wind_data takes away the "private" notation. Usually, we add the leading underscore when there are things happening on an attribute internal to the class that aren't fully supported from outside the class, and that's the case here as well. So, the more complete fix may be to make wind_data a public attribute and support setting it (and therefore the other atmospheric condition variables) from outside FlorisModel.

@misi9170 misi9170 merged commit a72b7fe into NREL:main Dec 19, 2024
13 checks passed
@misi9170 misi9170 deleted the bugfix/yaw-opt-warning branch December 19, 2024 06:18
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