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 environmental variables to driver input files #707

Merged
merged 31 commits into from
Oct 27, 2021

Conversation

hkross
Copy link
Contributor

@hkross hkross commented Apr 6, 2021

This pull request is ready to be merged.

Feature or improvement description

This pull request adds environmental variables to the AeroDyn, HydroDyn, and OpenFAST driver input files. Environmental variables in the AeroDyn and HydroDyn primary input files now have a "default" option. If set to "default", values are read in from the driver files. Otherwise, driver values are overwritten. An "MHK" switch is also added to the AeroDyn and OpenFAST driver input files to allow either AirDens or WtrDens to be read in as the density of the primary working fluid. For now, an error is thrown if the MHK switch is activated.

Impacted areas of the software

  • AeroDyn and AeroDyn driver
  • HydroDyn and HydroDyn driver
  • ElastoDyn
  • OpenFAST glue code

Test results

All regression tests have been updated and pass.

Check list

@ebranlard
Copy link
Contributor

The changes looks good to me. I'm not sure what the difference between MHK fixed and MHK floating would imply and if this distinction is needed for the driver.

@hkross
Copy link
Contributor Author

hkross commented Apr 13, 2021

The changes looks good to me. I'm not sure what the difference between MHK fixed and MHK floating would imply and if this distinction is needed for the driver.

Thanks, Emmanuel. The difference between fixed and floating is in how the coordinate system is handled, since a floating system will have its base above its tower. We will likely need this distinction for the driver so we can properly calculate submergence depth for both cases. We haven't implemented the floating case yet, so if it turns out the distinction is not needed, we can remove it later on.

@hkross hkross force-pushed the feature/Environmental_Variables branch from 98707b7 to 1d84d90 Compare May 5, 2021 01:08
@hkross
Copy link
Contributor Author

hkross commented Sep 27, 2021

@rafmudaf @andrew-platt @ebranlard This pull request is ready to merge, pending review and a final run of the regression tests.

Copy link
Contributor

@ebranlard ebranlard left a comment

Choose a reason for hiding this comment

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

The changes looks good to me.

I've noticed that WtrDens was changed from SiKi to ReKi in the Waves module. This could potentially change results. An alternative would be to keep it as SiKi, and then do a type conversion when this variable is set ( wave%WtrDens = real( input%WtrDens , SiKi ) ) .

@hkross
Copy link
Contributor Author

hkross commented Sep 29, 2021

The changes looks good to me.

I've noticed that WtrDens was changed from SiKi to ReKi in the Waves module. This could potentially change results. An alternative would be to keep it as SiKi, and then do a type conversion when this variable is set ( wave%WtrDens = real( input%WtrDens , SiKi ) ) .

@andrew-platt Do you have a preference on how this is done?

@andrew-platt
Copy link
Collaborator

If all the other variables are handled as ReKi at the input, it is might be cleaner reading code to do the type conversion when setting it for the waves module as @ebranlard suggested.

I don't have a strong opinion either way.

@hkross
Copy link
Contributor Author

hkross commented Sep 30, 2021

Thanks, @ebranlard and @andrew-platt. I will make the suggested change.

@rafmudaf rafmudaf added this to the v4.0.0 milestone Oct 25, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #707 (9044b91) into dev (0789ac2) will increase coverage by 0.01%.
The diff coverage is 25.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #707      +/-   ##
==========================================
+ Coverage   11.29%   11.30%   +0.01%     
==========================================
  Files         211      211              
  Lines      310557   310787     +230     
  Branches   181045   178127    -2918     
==========================================
+ Hits        35068    35137      +69     
- Misses     231395   231510     +115     
- Partials    44094    44140      +46     
Flag Coverage Δ
regtests 11.19% <25.69%> (+0.01%) ⬆️
unittests 2.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/aerodyn/src/AeroDyn_Driver_Types.f90 0.99% <0.00%> (-0.01%) ⬇️
modules/elastodyn/src/ElastoDyn_IO.f90 26.12% <ø> (-0.06%) ⬇️
modules/elastodyn/src/ElastoDyn_Types.f90 12.48% <0.00%> (+<0.01%) ⬆️
modules/aerodyn/src/AeroDyn_Types.f90 11.97% <5.17%> (-0.04%) ⬇️
modules/hydrodyn/src/HydroDyn_Types.f90 7.79% <16.66%> (+0.02%) ⬆️
modules/hydrodyn/src/HydroDyn_DriverCode.f90 27.93% <25.00%> (-0.15%) ⬇️
modules/openfast-library/src/FAST_Subs.f90 19.58% <32.35%> (+0.42%) ⬆️
modules/openfast-library/src/FAST_Types.f90 8.05% <33.33%> (+0.04%) ⬆️
modules/aerodyn/src/AeroDyn_Driver_Subs.f90 29.90% <52.00%> (+0.15%) ⬆️
modules/aerodyn/src/AeroDyn.f90 27.18% <54.54%> (+0.06%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0789ac2...9044b91. Read the comment docs.

@hkross
Copy link
Contributor Author

hkross commented Oct 26, 2021

@rafmudaf @ebranlard @andrew-platt All of the tests are now passing, so this pull request is ready to merge.

Copy link
Collaborator

@andrew-platt andrew-platt left a comment

Choose a reason for hiding this comment

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

I don't see the input file changes for the main .fst file listed in the docs/source/user/api_change.rst. Could you add a couple lines there in the first table with the input file changes?

@hkross
Copy link
Contributor Author

hkross commented Oct 27, 2021

I don't see the input file changes for the main .fst file listed in the docs/source/user/api_change.rst. Could you add a couple lines there in the first table with the input file changes?

@andrew-platt Done!

@andrew-platt andrew-platt changed the base branch from dev to main October 27, 2021 23:10
@andrew-platt andrew-platt changed the base branch from main to dev October 27, 2021 23:10
@andrew-platt andrew-platt merged commit 572ff1a into OpenFAST:dev Oct 27, 2021
@rafmudaf rafmudaf mentioned this pull request Mar 2, 2022
11 tasks
@rafmudaf rafmudaf modified the milestones: v4.0.0, v3.1.0 Jun 1, 2022
@hkross hkross deleted the feature/Environmental_Variables branch October 25, 2022 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants