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

Bug fix: Convert layout to layout_x and layout_y in FlorisInterface.reinitalize #470

Merged
merged 7 commits into from
Jul 27, 2022

Conversation

RHammond2
Copy link
Collaborator

@RHammond2 RHammond2 commented Jul 22, 2022

Feature or improvement description
This PR modifies FlorisInterface.reinitialize() to transition the layout parameter to be layout_x and layout_y and all reliant functionality. This resembles the behavior within FLORIS itself, and clears up the confusion surrounding what the correct dimensions of layout should be and accidental truncating of a farm's layout.

Related issue, if one exists
#413

Impacted areas of the software
floris.tools.floris_interface.FlorisInterface
floris.tools.optimization.pyoptparse.layout.Layout
floris.tools.uncertainty_interface.UncertaintyInterface

Additional supporting information
n/a

Test results, if applicable
All tests pass, though there aren't any tests for this class as it stands.

@RHammond2 RHammond2 added bug Something isn't working floris.tools labels Jul 22, 2022
@RHammond2 RHammond2 requested a review from rafmudaf July 22, 2022 22:46
@RHammond2 RHammond2 linked an issue Jul 22, 2022 that may be closed by this pull request
@RHammond2 RHammond2 changed the title Bug fix/413 layout Bug fix: Convert layout to layout_x and layout_y in FlorisInterface.reinitalize Jul 25, 2022
@paulf81
Copy link
Collaborator

paulf81 commented Jul 26, 2022

I agree with this change, thank you @RHammond2

My only thought, to not brake legacy code, what do you and @rafmudaf and @bayc think about allowing still the layout to be passed in (and a small block that parses it back to layout_x and layout_y in that case)? We can add a deprecation warning that we'll phase it out completely in 3.3 or something?

@rafmudaf
Copy link
Collaborator

With layout available again, this doesn't truly fix #413, right?

@RHammond2
Copy link
Collaborator Author

@rafmudaf, I think that implementing deprecation warning and encouraging the use of layout_x and layout_y in place of layout, in addition to changing all the examples so they rely on the use of layout_x and layout_y, should help transition users into not using the layout parameter. I'm happy to defer to you if you think we should implement the change immediately, but realizing that this would break users' code, giving a minor release in between breakage seemed kinder.

@paulf81
Copy link
Collaborator

paulf81 commented Jul 26, 2022

You're right @rafmudaf in theory that issue will not be resolved until we cease allowing layout, but I agree having a minor release with a deprecation warning seems kinder

@rafmudaf
Copy link
Collaborator

That sounds good to me, as well. In that case, @RHammond2 I've removed the "closes" word in your original post so that GitHub doesn't automatically close that issue.

@RHammond2
Copy link
Collaborator Author

That works for me to not close this. Should we tag this with a new v3.3 tag to remind ourselves of it?

@rafmudaf
Copy link
Collaborator

Yes very good idea!

@paulf81
Copy link
Collaborator

paulf81 commented Jul 27, 2022

Agree, starting to flag some things for v3.3 sounds great!

@paulf81 paulf81 merged commit cb1f54f into NREL:develop Jul 27, 2022
@RHammond2 RHammond2 deleted the bug_fix/413_layout branch July 27, 2022 20:24
@rafmudaf rafmudaf mentioned this pull request Sep 12, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working floris.tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What should reinitialize layout do in this case?
3 participants