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

Update uncertainty interface to 4d, new API #821

Merged
merged 37 commits into from
Mar 7, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Feb 22, 2024

Update uncertainty interface to 4d, new API

This pull request updates UncertaintyInterface to the v4 4d structure and the new set/run api currently in the fi_api branch. In doing so it resolves Issue #516 since uncertainty will now work equivalently to time series or wrapped wind rose.

The UncertaintyInterface, unlike previous implementatons, inherits the FlorisInterface and overloads certain functions to implement wind direction uncertainty (in practice this was the only type used). As before it uses gaussian averages of perturbations, but makes greater use of rounding and numpy reshaping and summing methods to achieve this result. It reimplements set (leaving run as is), as well as get_turbine_powers, get_farm_power, get_farm_aep to return this uncertain result. Parameters of uncertainty are passed in at initialization.

Pull request includes unit tests of some of the underlying functions, and an update of example 20_ to show the new usage.

Related issue

#516

Impacted areas of the software

uncertainty_interface.py

@paulf81 paulf81 added enhancement An improvement of an existing feature v4 Focus of FLORIS v4 labels Feb 22, 2024
@paulf81 paulf81 added this to the v4.0 milestone Feb 22, 2024
@paulf81 paulf81 self-assigned this Feb 22, 2024
often does not perfectly know the true wind direction, and that a
turbine often does not perfectly achieve its desired yaw angle offset.
Defaults to fix_yaw_in_relative_frame=False.
wd_resolution (float, optional): The resolution of wind direction, in degrees.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not here, but in general we should pick either "wind_direction" or "wd" for wind direction named arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And same for the other related input arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I would be happy either way, with maybe a slight preference toward wd_ for succinctness in this case

@rafmudaf
Copy link
Collaborator

Thanks for addressing my comments @paulf81. Overall, this is looking pretty good to me.

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 28, 2024

@rafmudaf and @misi9170 , just to close a loop here, @misi9170 added some tests of applying yaw angles to the uncertainty interface that threw some unexpected errors. I tracked down two seperate issues:

  1. One is a problem of passing lists, or integers, to yaw_angles in the set function to FlorisInterface. I made a new pull request for handling that one ([Bugfix] Cast yaw angles to np.ndarray on set #828 )

  2. I realized that having one saved internal floris_interface was a bug, because in some places I expected it to hold the un_expanded conditions and in others the expanded. This works fine if you call set once, but falls apart after that. However, the solution of holding two seperate fis, with the expanded created every time you run _set_uncertain, I think clears everything up. This change is now in the code.

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 1, 2024

Hi @rafmudaf and @misi9170 , we talked today about possibly removing the un-expanded FLORIS interface, I started making the changes tonight (storing the dict instead) but there did seem to be some trade-offs, mainly I think in the set function the operation would be to declare a new FLORIS object, and again only store the dict. I think it clearly could be done, but if you wouldn't mind having a quick look and concurring it should be done (versus the current version's memory usage but clearer syntax), let me know what you think and I'll make the change

@paulf81 paulf81 requested a review from rafmudaf March 1, 2024 04:54
@misi9170
Copy link
Collaborator

misi9170 commented Mar 3, 2024

Hi @rafmudaf and @misi9170 , we talked today about possibly removing the un-expanded FLORIS interface, I started making the changes tonight (storing the dict instead) but there did seem to be some trade-offs, mainly I think in the set function the operation would be to declare a new FLORIS object, and again only store the dict. I think it clearly could be done, but if you wouldn't mind having a quick look and concurring it should be done (versus the current version's memory usage but clearer syntax), let me know what you think and I'll make the change

I guess an option would be to create a dict direction rather than instantiating a new FLORIS object and then exporting it to a dict. The code for that (building a floris_dict) is in FlorisInterface._reinitialize(). It doesn't seem great to copy that code into UncertaintyInterface, but we could consider having a standalone function that builds a floris_dict that both FlorisInterface._reinitialize() and UncertaintyInterface.set() call. But, honestly, that might be overdoing it; I'd say that, for now, it's probably fine to just go ahead and instantiate a FLORIS object (and allocate memory to it); and then convert it to a dict with .as_dict() and (presumably) release the memory again. It's not super elegant, but at least it's easy with the methods we already have in place.

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 4, 2024

Hi @rafmudaf and @misi9170 , we talked today about possibly removing the un-expanded FLORIS interface, I started making the changes tonight (storing the dict instead) but there did seem to be some trade-offs, mainly I think in the set function the operation would be to declare a new FLORIS object, and again only store the dict. I think it clearly could be done, but if you wouldn't mind having a quick look and concurring it should be done (versus the current version's memory usage but clearer syntax), let me know what you think and I'll make the change

I guess an option would be to create a dict direction rather than instantiating a new FLORIS object and then exporting it to a dict. The code for that (building a floris_dict) is in FlorisInterface._reinitialize(). It doesn't seem great to copy that code into UncertaintyInterface, but we could consider having a standalone function that builds a floris_dict that both FlorisInterface._reinitialize() and UncertaintyInterface.set() call. But, honestly, that might be overdoing it; I'd say that, for now, it's probably fine to just go ahead and instantiate a FLORIS object (and allocate memory to it); and then convert it to a dict with .as_dict() and (presumably) release the memory again. It's not super elegant, but at least it's easy with the methods we already have in place.

Ok @misi9170 , I made the change, now the unexpanded FI is only created as needed and only the dictionary version is saved. One place we might just want to double check ourselves, does the dictionary save the values of yaw_angles/power_setpoints? If not repeated calls to uncertainity_interface.set() might work a little differently in this version than the previous where we hold the entire fi in some cases, but I'm not sure,

@misi9170
Copy link
Collaborator

misi9170 commented Mar 4, 2024

@paulf81 no, the dictionary doesn't hold the yaw_angles and power_setpoints; unless those are saved elsewhere, they'll be lost, so we might need to put in special handling for that since it would be nice if that was consistent between FlorisInterface and UncertaintyInterface. See here, where the setpoints are "replayed" over the newly instantiated FLORIS object. That won't work directly in the UncertaintyInterface, but they could be saved on hidden attributes until they are needed (if they are needed).

@paulf81 paulf81 force-pushed the feature/update_uncertain_interface branch from 8ff10ca to 5269bc6 Compare March 6, 2024 03:52
@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 6, 2024

ok @rafmudaf and @misi9170 , tonight it:

  1. Reverted this back to using a seperate FlorisInterface (rather than dict, a little wasteful but safer)
  2. I merged v4 back in following the merge into v4 of the change to layout_visualization.
  3. Made sure I had addressed previous hanging comments

Think it's now ready,

@NREL NREL deleted a comment from misi9170 Mar 6, 2024
@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 7, 2024

@rafmudaf good to merge this one?

@paulf81 paulf81 merged commit ef87def into NREL:v4 Mar 7, 2024
8 checks passed
@paulf81 paulf81 deleted the feature/update_uncertain_interface branch March 7, 2024 18:09
@misi9170 misi9170 mentioned this pull request Apr 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature v4 Focus of FLORIS v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants