-
Notifications
You must be signed in to change notification settings - Fork 155
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
Feature: Add external library functionality #521
Feature: Add external library functionality #521
Conversation
The tests are passing locally, but I'm getting the following error when running in the CI Pipeleine. @rafmudaf do you know how to correctly configure the
|
Hey @RHammond2 I see you have "nrel_5mw" in the test case but the file is "nrel_5MW.yaml". Ubuntu is cases sensitive but macOS is not, so that may be the reason for the inconsistency on your computer vs GitHub Actions. |
@RHammond2 , the change itself looks good to me! Thanks for getting this implemented. I think this is better than having the user specify the full absolute or relative path to the turbine files with the |
Good idea @bayc. Additionally, I think this implementation requires to use either the user-supplied library or the default library but not both. Is it possible to include support for using both? This would allow using the NREL 5MW as well as a custom turbine definition without having to redefine the NREL 5MW, for example. |
Also @RHammond2 the codecov upload step has been failing for me, but I have not taken the time to understand why. It seems this happens from time to time, and I'm leaning toward just turning off codecov since I personally never look at it and probably no one else does either. |
This same issue has been plaguing FLASC, some googling and it seems to be something between github.com and codecov so also want to remove it from FLASC, so if we take the decision here would like to know how it's done |
@rafmudaf thanks for clarifying the confusion on the code coverage report. If it's only failing sometimes, I think I'm ok with merging if it's only that last step failing and ignoring it, but if it's always failing, then I'd say let's take it out and we can figure this out another way possibly. On further inspection, in Openoa we do a similar CI step: https://github.com/NREL/OpenOA/blob/main/.github/workflows/ci-tests.yml#L36-L37 with the major difference being that we don't use this line, which is present in FLORIS: https://github.com/NREL/floris/blob/main/.github/workflows/continuous-integration-workflow.yaml#L51. So it seems like we could just remove that line and the CI integration won't fail if it can't do the bonus step of uploading the coverage report. |
…lize() for no turbine redefinition
See #568 |
Feature or improvement description
This PR adds support for using external turbine libraries for turbine defintions to
floris.tools.FlorisInterface
andfloris.simulation.Farm
.Related issue, if one exists
This would bump up the priority of #345 and likely close it.
Impacted areas of the software
floris.simulation.Farm
now has aturbine_library
attribute that defaults to the currently assumed "floris/turbine_library" folder that ensures the input file path exists prior to loading in the data. Additionally this input was added to thefloris.tools.FlorisInterface.reinitialize()
method to ensure usage is consistent with the rest of the inputs. I've also added a few other tests and some test data to ensure this is working as expected.Additional supporting information
I saw in #345 that this was marked as v3.3/v3.4 issue, however, in a separate project I'm running into an issue where the current workaround is a little bit clunky in practice, and am proposing the implemented solution here as a starting point.
Test results, if applicable
All previously existing and newly added tests pass.