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] Cast yaw angles to np.ndarray on set #828

Merged
merged 6 commits into from Mar 6, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Feb 28, 2024

[Bugfix] Fix yaw setting

In the course of some other work, I noticed that a few conditions that I would've expected to be ok throw an error:

  • Passing a list to yaw_angles in fi.set
  • If the values of yaw_angles are integers instead of floats

This is not so hard to fix but I wasn't positive the preferred solution (attrs?). So I thought I'd start just by adding the failing tests and then we can agree on how we want to fix it.

Other things to consider:

  • Does this also impact power setpoints? No, tested
  • Does this also impact disable turbines? No, tested
  • Does fixing this in FlorisInterface intrinsically fix the problem in Parallel and Uncertain interface? It seems to, yes. I tried this on the Update uncertainty interface to 4d, new API #821 (implemented the fixed, and then altered the test to take yaw angles as a list), and it worked.

Note this issue is a little bit being worked-around in #821

@paulf81 paulf81 added the bug Something isn't working label Feb 28, 2024
@paulf81 paulf81 added this to the v4.0 milestone Feb 28, 2024
@paulf81 paulf81 self-assigned this Feb 28, 2024
@rafmudaf
Copy link
Collaborator

rafmudaf commented Feb 29, 2024

@misi9170
Copy link
Collaborator

misi9170 commented Mar 6, 2024

The fix was straightforward here---in _set_operation(), I've now wrapped the assignment of the yaw_angles onto self.floris.farm.yaw_angles in a np.array() statement.

Incidentally, this is something that @rafmudaf warned me about---that reaching in and altering the attributes (rather than instantiating them properly, which would pass through __attrs__) can lead to unintended consequences. For now, this fix works; but we should possibly consider adding setter methods to Farm to avoid this happening again.

@misi9170 misi9170 marked this pull request as ready for review March 6, 2024 02:40
@misi9170
Copy link
Collaborator

misi9170 commented Mar 6, 2024

@paulf81 I can't add you as a reviewer, since you opened the PR, but please look through and see what you think.

@misi9170 misi9170 self-assigned this Mar 6, 2024
@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 6, 2024

This works great for me, thank you @misi9170 I "approve"

@@ -355,7 +355,7 @@ def _set_operation(
"""
# Add operating conditions to the floris object
if yaw_angles is not None:
self.floris.farm.yaw_angles = yaw_angles
self.floris.farm.yaw_angles = np.array(yaw_angles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the only place where yaw angles are going to be set? If not, you might consider adding a setter method on Farm to do this always:

class Farm:
    def set_yaw_angles(yaw_angles):
        # Do lots of checks
        ...
        # Cast to Numpy array
        self.yaw_angles = np.array(yaw_angles)

You could also use the @property decorator the way we used to do, if you think maintaining the attribute-style use is important.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, there's already a function with this name at https://github.com/NREL/floris/blob/v4/floris/simulation/farm.py#L332

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah and I just read #828 (comment), sorry to say the same thing @misi9170

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem---I'm going to try to do this (use a setter method), it seems like a better approach

@rafmudaf rafmudaf self-requested a review March 6, 2024 17:37
@rafmudaf rafmudaf changed the title [Bugfix] Fix yaw setting [Bugfix] Cast yaw angles to np.ndarray on set Mar 6, 2024
@misi9170 misi9170 merged commit a60060c into NREL:v4 Mar 6, 2024
8 checks passed
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
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants