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

Registry: add flag to allow integers in InputType #1999

Conversation

andrew-platt
Copy link
Collaborator

This is ready to merge.

Feature or improvement description
PR #1973 modified the registry to check for invalid types in the InputType. However, the ExtLoads registry includes integers in the InputType. We really don't want integers in that derived type as it breaks with linearization.

Rather than change the ExtLoads registry and potentially break downstream codes that depend on it (AMR-Wind for example), I propose we allow an exception here. Unless I add such an exception, or force downstream changes, we cannot merge the dev branch into the dev-unstable-pointers branch.

I added a flag -inputintallow to the registry for use in such special cases.

Impacted areas of the software
Registry only.

Test results, if applicable
None.

@andrew-platt
Copy link
Collaborator Author

@bjonkman, do you have an opinion on this approach?

@bjonkman
Copy link
Contributor

One reason for not having integers in the Input type is because of the extrapolation/interpolation routines. Are you omitting those routines from this module, too?

@bjonkman
Copy link
Contributor

Also, that PR didn't ADD a check on the invalid type, it just printed the error message before ending. Otherwise, the Registry would end without an error message and without completing.

{
std::cerr << "Registry warning: Data type '" << dt_name << "' contains non-real values." << std::endl;
exit(EXIT_FAILURE);
if ((ddt.interface != nullptr) && ddt.interface->only_reals)
Copy link
Collaborator

@deslaughter deslaughter Jan 22, 2024

Choose a reason for hiding this comment

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

Instead of adding a new flag, you could disable this check if the -noextrap flag has been set: if ((ddt.interface != nullptr) && ddt.interface->only_reals && !this->no_extrap_interp) and then add that flag to for this module. I didn't see extrap/interp being called for ExternalInflow. However, I also don't think there should be an integer in the InputType.

@bjonkman
Copy link
Contributor

@andrew-platt , The integers that I see in that InputType for ExtLoadsDX seem like they should be parameters or initialization inputs.
image

I know it's more work to fix that module (and the interface to CFD), but I think in the long term, it will be better to make the ExtLoads module align with the OpenFAST framework as much as possible.

@andrew-platt
Copy link
Collaborator Author

After looking at this a bit more, I agree with @bjonkman and @deslaughter we should simply not allow integers in the InputType regardless. These particular values really are parameters and should be treated as such. In the long run it will be better to fix this properly and align the ExtLoads module with the framework.

There also appear to be some additional issues to resolve from PR #1932, so we will address this along those issues in a different PR.

So, I'm going to close this PR without merging.

@andrew-platt andrew-platt deleted the f/RegistryAlowInputInts branch January 22, 2024 17:28
@andrew-platt
Copy link
Collaborator Author

Better solution than this PR: #2001

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.

3 participants