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

Add merge/reduce FLORIS objects #866

Merged
merged 123 commits into from
Apr 8, 2024
Merged

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Apr 4, 2024

Just a rough draft for now @misi9170 and @Bartdoekemeijer of sketching in merging/reducing FLORIS objects (taking these functions out of FLASC and into FLORIS), we'll look a mess until we merge refactor...

Then also, first decision, is this the right place? Do we prefer a static member function?

@paulf81 paulf81 added the v4 Focus of FLORIS v4 label Apr 4, 2024
@paulf81 paulf81 added this to the v4.0 milestone Apr 4, 2024
@paulf81 paulf81 requested a review from misi9170 April 4, 2024 17:48
@misi9170
Copy link
Collaborator

misi9170 commented Apr 5, 2024

I think this way (as a static function in floris_model.py) is reasonable. Will we want to do the same for uncertain models and parallel models? perhaps we can figure out a convenient function that could handle any of them? In that case, we may want this function on a separate file.

I'm more into the merge function than the reduce function. For reducing, we already have the following options:

  • Updating layout_x and layout_y in set()
  • Using disable_turbines for "temporarily" reducing (although the computation will still be "large").
    I guess I feel that things could easily go wrong with the reduction function

@paulf81
Copy link
Collaborator Author

paulf81 commented Apr 5, 2024

I think this way (as a static function in floris_model.py) is reasonable. Will we want to do the same for uncertain models and parallel models? perhaps we can figure out a convenient function that could handle any of them? In that case, we may want this function on a separate file.

I'm more into the merge function than the reduce function. For reducing, we already have the following options:

  • Updating layout_x and layout_y in set()
  • Using disable_turbines for "temporarily" reducing (although the computation will still be "large").
    I guess I feel that things could easily go wrong with the reduction function

Ok sounds good, I'll set it as a static member of the FlorisModel and remove the reduce function. I think for uncertain/parallel these should be defined specifically for those, rather than trying to make a catch-all for now, which we could do later, but that moment catching all the special cases could be something. At any rate I think I will just make the change in FlorisModel and then pause and wait for refactor merge just to simplify a little

@paulf81
Copy link
Collaborator Author

paulf81 commented Apr 5, 2024

nevermind... hit the wrong button

@paulf81 paulf81 closed this Apr 5, 2024
@paulf81 paulf81 reopened this Apr 5, 2024
@paulf81 paulf81 marked this pull request as ready for review April 6, 2024 04:20
@paulf81
Copy link
Collaborator Author

paulf81 commented Apr 6, 2024

Ok @misi9170 , I think this one is ready, I think for now just adding the function is good. I tested it out a bit with some test code, but I think for now we won't call it in the examples and can add that later (or just make greater use in flasc). This is ready from my perspective

if len(fi_turbine_type) == 1:
fi_turbine_type = fi_turbine_type * len(fmodel.layout_x)
elif not len(fi_turbine_type) == len(fmodel.layout_x):
raise UserWarning("Incompatible format of turbine_type in fmodel.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these UserWarnings be errors?

if reference_wind_height is None:
reference_wind_height = np.mean(reference_wind_heights)
if np.any(np.abs(np.array(reference_wind_heights) - reference_wind_height) > 1.0e-3):
raise UserWarning(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above

y_list = []
turbine_type_list = []
reference_wind_heights = []
for fmodel in fi_list:
Copy link
Collaborator

Choose a reason for hiding this comment

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

fmodel.reset_operation()

@misi9170
Copy link
Collaborator

misi9170 commented Apr 7, 2024

@paulf81 I made a few changes in d127c70

  • Changed UserWarning to ValueError in a few places (please feel free to change back if you intended to raise UserWarnings)
  • Changed fi and FlorisInterface to fmodel and FlorisModel throughout, and made warning about not providing FlorisModels more general
  • Included a reset_opeartion() call so that any control setpoints, which are specified for a specific number of turbines, are dropped.

If you're happy with those changes, feel free to merge this in.

@misi9170 misi9170 self-requested a review April 7, 2024 21:49
@misi9170 misi9170 merged commit 27b67ab into NREL:v4 Apr 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Focus of FLORIS v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants