Skip to content

Remove deprecated user API's#603

Merged
rafmudaf merged 5 commits intoNatLabRockies:developfrom
rafmudaf:remove_deprecated
May 4, 2023
Merged

Remove deprecated user API's#603
rafmudaf merged 5 commits intoNatLabRockies:developfrom
rafmudaf:remove_deprecated

Conversation

@rafmudaf
Copy link
Collaborator

@rafmudaf rafmudaf commented Mar 7, 2023

Remove user interface elements marked deprecated or to-be-removed

This pull request removes user-facing functions that were deprecated in v3 or v3.1 (see #488). All infrastructure has already dealt with these changes and they were left in place as a convenience to users.

Related issue

Impacted areas of the software

This impacts interfaces in Floris.tools and the turbine definition.

Additional supporting information

The motivation for this is mostly to maintain a clean and consistent API. These could all be left in place, but the decisions to move away from them were made at one point for specific reasons. Rather than continuing to accumulate various warning messages and if-statements, I'd rather follow through with the intent and fully remove these functions acknowledging that it might cause headaches for some users.

@rafmudaf rafmudaf self-assigned this Mar 7, 2023
@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Mar 7, 2023

FWIW This could easily wait until v3.4 since that release is planned to have larger user-facing impacts.

@paulf81
Copy link
Collaborator

paulf81 commented Mar 7, 2023

I agree with the spirit of this, and think we should do it, alongside 3.4 makes a lot of sense

One idea I had is we could do something like:

  • Grab an input example from 3.1 (or .0 or .2)
  • Try to run it in 3.4 and note all the faults
  • Make a website that explains why each of the faults appeared and the simple remediation
  • Link to the website when the fault gets thrown

What do you think?

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Mar 7, 2023

@paulf81 I like it. As a starting point, we have a similar page to communicate input file changes in OpenFAST here. It's a bit verbose but the OpenFAST input files themselves are verbose.

@rafmudaf rafmudaf mentioned this pull request Mar 7, 2023
6 tasks
@paulf81
Copy link
Collaborator

paulf81 commented Mar 8, 2023

That sounds good, I think if we did the same with FLORIS like you say, it would be smaller so would be hopefully not too big of a lift,

@rafmudaf rafmudaf force-pushed the remove_deprecated branch from 56bb8f3 to ed98f16 Compare April 26, 2023 17:03
@rafmudaf
Copy link
Collaborator Author

rafmudaf commented May 3, 2023

@RHammond2 could I get your help here on the test_farm_unique_loading test for the Farm object? There are no longer errors for the x_20MW turbine model, so we could remove the assertion. However, I'm not sure what is a better test, so could you recommend a good check that this function works?

@RHammond2
Copy link
Collaborator

@rafmudaf, I think that there isn't really a great way to test this functionality at present, so I would vote for removing test_farm_unique_loading, which was intended to prove that we're catching the warning only one time. Without that piece of code there is no way to verify how many times a file is loaded.

As a more general note, because that check is removed, the x_20MW.yaml should fail to load wherever it's used because there is no ref_density_cp_cts default value.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented May 4, 2023

As a more general note, because that check is removed, the x_20MW.yaml should fail to load wherever it's used because there is no ref_density_cp_cts default value.

Ok sounds good to me. I think an error is a good result for a missing input, at this point.

The error message is removed in a previous commit so the test is no longer relevant
@RHammond2
Copy link
Collaborator

Ok sounds good to me. I think an error is a good result for a missing input, at this point.

I'm just realizing that this means example 18 should probably fail, as well as 18b whenever that's brought in.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented May 4, 2023

The last commit should take care of that

@rafmudaf rafmudaf requested a review from paulf81 May 4, 2023 19:59
@rafmudaf rafmudaf added this to the v3.4 milestone May 4, 2023
@rafmudaf
Copy link
Collaborator Author

rafmudaf commented May 4, 2023

@paulf81 we discussed capturing the changes to the input file per #603 (comment). I like it and it should certainly be added. Let's keep this pull request with the current scope and add those changes in a subsequent pr.

@paulf81
Copy link
Collaborator

paulf81 commented May 4, 2023

@paulf81 we discussed capturing the changes to the input file per #603 (comment). I like it and it should certainly be added. Let's keep this pull request with the current scope and add those changes in a subsequent pr.

Agreed!

Copy link
Collaborator

@paulf81 paulf81 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this cleanup @rafmudaf !! It's great to keep pruning the dead wood out, thank you for doing this!

@rafmudaf rafmudaf merged commit 6c4f70f into NatLabRockies:develop May 4, 2023
@rafmudaf rafmudaf deleted the remove_deprecated branch May 6, 2023 17:45
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.

3 participants

Comments