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

Inconsistent handling for optional input parameters without a default value #24455

Closed
lingzou opened this issue May 23, 2023 · 7 comments · Fixed by #25245
Closed

Inconsistent handling for optional input parameters without a default value #24455

lingzou opened this issue May 23, 2023 · 7 comments · Fixed by #25245
Assignees
Labels
C: Framework P: minor A defect that does not affect the accuracy of results. T: defect An anomaly, which is anything that deviates from expectations.

Comments

@lingzou
Copy link
Contributor

lingzou commented May 23, 2023

Reason

In MOOSE, an optional input parameter could be provided without default values, e.g.,

params.addParam<Real>("dummy_real", "a dummy real number");

When handling such an input parameter, it is typical to check if it is valid before accessing it, e.g., the following to avoid runtime error:

_dummy_real(isParamValid("dummy_real") <- Wrong, runtime error: The parameter "dummy_real" is being retrieved before being set.

The correct example is:
_dummy_real(isParamValid("dummy_real") ? getParam<Real>("dummy_real") : 0.0)

However, if the input parameter is a std::vector, e.g.,

params.addParam<std::vector<unsigned int>>("dummy_vec", "dummy_vec");
_dummy_vec(getParam<std::vector<unsigned int>>("dummy_vec"))

MOOSE won't send out a runtime error, but of course, _dummy_vec is an empty vector. This is inconsistent with the first case. This probably should not be categorized as a bug, would still be nice to make things consistent.

Design

For the second case described above for std::vector input parameter, if no default value provided, should be handled the same way as other cases such as a real number.

Impact

Inconsistent code design; causing confusion in module/application development.

@lingzou lingzou added the T: task An enhancement to the software. label May 23, 2023
@GiudGiud GiudGiud added C: Framework T: defect An anomaly, which is anything that deviates from expectations. P: minor A defect that does not affect the accuracy of results. and removed T: task An enhancement to the software. labels Jun 10, 2023
@GiudGiud GiudGiud moved this to Todo in NEAMS MP TA 23 Jun 26, 2023
@MengnanLi91
Copy link
Contributor

@GiudGiud @lindsayad @friedmud @dschwen @permcody @loganharbour I added a runtime error for empty vector check in my PR #25245 but it breaks tons of test cases because of the new error messages. Any suggestions on how to fix those failed tests? I can simply add a default value. Or treat them differently based on objects? @loganharbour suggests to start a discussion on this issue.

@loganharbour
Copy link
Member

For context - if you add a parameter for anything but a std::vector without a value and call getParam on it, you will get a "retrieved before being set" error. We did this on purpose - but why? It's not consistent

@GiudGiud
Copy link
Contributor

I think adding a default is fine where it makes sense, and for others you ll want to fix the broken logic
Hit up the respective SMEs if in doubt. Or git blame to find someone

This "retrieved before being set" is the right behavior

@loganharbour
Copy link
Member

This "retrieved before being set" is the right behavior

I would agree with you, but clearly someone else didn't:

* when returning most scalar types, but will allow retrieving empty vectors.

And that's the important conversation here

@GiudGiud
Copy link
Contributor

@permcody did you do this and why

@permcody
Copy link
Member

I can't recall of a specific reason for this other than making an assumption that an empty vector was harmless... I'm supportive of making the behavior consistent.

@dschwen
Copy link
Member

dschwen commented Aug 27, 2023

We are relying on this behavior heavily (as the failing tests show). I'm fine with changing it, as long as fixes for all apps are part of the "service package" ;-)

Adding an empty default used to be a bit cumbersome, but nowadays one can probably simply add {}.

@github-project-automation github-project-automation bot moved this from In Progress to Done in NEAMS MP TA 24 Nov 22, 2023
GiudGiud added a commit to GiudGiud/moose that referenced this issue Nov 22, 2023
GiudGiud added a commit to GiudGiud/moose that referenced this issue Nov 28, 2023
…that do not have a source variable) in conservative transfers, refs idaholab#24455
pbehne pushed a commit to pbehne/moose that referenced this issue Dec 1, 2023
pbehne pushed a commit to pbehne/moose that referenced this issue Dec 1, 2023
…that do not have a source variable) in conservative transfers, refs idaholab#24455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework P: minor A defect that does not affect the accuracy of results. T: defect An anomaly, which is anything that deviates from expectations.
Projects
Status: Done
Status: Todo
Development

Successfully merging a pull request may close this issue.

6 participants