Clean up NEML coupling material #194
Conversation
dschwen
commented
Mar 31, 2021
dschwen
commented
- make sure types match
- make things constant
- fix index types
- permit supplying XML substitution values as a vector parameter
- simplify and extend error checking (detect duplicate inames)
- substitute all occurrences of a {variable} (and simplify substitution code)
- don't pass around stuff as parameters that should be members
|
Ugh, this is borked. There is an error check that makes sure all {variables} in the XML (for substitution) get a value assigned in the input ( In the old code this error check was only run if at least one However this fails a few inputs now. The main issue is that each XML can contain multiple models, and we actually should not care about {variable} substitution in the models we are not using. However that would require parsing the XML to perform the checks only in the XML block that corresponds to the model we're using. Alternatively we add functionality to NEML to register the substitutions in a map that is uses at parse time in NEML. In that case NEML would perform the necessary error checking and we didn't have to parse the XML twice. |
|
To get this to pass I'll disable the error checking for the no inames case for now. Even though I hate that. |
|
@bwspenc could I get a review, please |
|
|
||
| void replaceXmlVariables(std::string & xmlStringForNeml) const; | ||
| /// Substitute values in for the variables names found in the string version of the xml file |
There was a problem hiding this comment.
Have we dropped the standard of documenting the methods with the /** doxygen comments? I personally don't like it, but I thought that was the MOOSE coding standard. I went back and looked, and it doesn't seem to be explicitly stated that we need that anymore: https://mooseframework.inl.gov/framework_development/code_standards.html
It used to stipulate that we use those, didn't it?
There was a problem hiding this comment.
/// are single line doxygen comments!
There was a problem hiding this comment.
I know, but I swear that the style guidelines used to require that we use multiline doxygen comments with @param, etc.
There was a problem hiding this comment.
Now it just says "Try to document as much as possible" (but gives a multiline doxygen example)!
src/materials/NEMLStress.C
Outdated
| for (size_t i = 0; i < _nvars_max; ++i) | ||
| { | ||
| auto istr = Moose::stringify(i); | ||
| params.addParam<Real>("neml_variable_value" + istr, |
There was a problem hiding this comment.
Maybe we don't even need these neml_variable_valueX parameters anymore. If I remember correctly, the only reason we added those was for running stochastic analyses, but now we have the ability to set components of a vector parameter.
Regardless, we'll need to deprecate them and keep them for a while, but maybe we can do a follow-on commit to do that once we verify that the new capability works here.
There was a problem hiding this comment.
oh, ok, if we can control vector entries these can be set to deprecated
There was a problem hiding this comment.
Pushed the deprecation up, but it seems I got the syntax for modifying vector entries wrong, so this will fail that one test.
There was a problem hiding this comment.
Fixed with help from Wen!
| " neml_variable_iname entries"); | ||
|
|
||
| // We permit the numbered parameters to override the values vector. | ||
| _neml_variable_value.resize(_nvars); |
There was a problem hiding this comment.
I guess it doesn't really hurt anything, but it kind of seems odd to resize _neml_variable_value and go into the loop below in the case when neml_variable_value is set.
There was a problem hiding this comment.
You mean when it is not set? In that case I'm not going into the loop, because the continuation condition in the head of the loop is never met. I never liked having extra conditionals around such loops, they just seem redundant.
| return str; | ||
| _xml.assign((std::istreambuf_iterator<char>(t)), std::istreambuf_iterator<char>()); | ||
|
|
||
| // replace {variables} in the XML file (the _nvars > 0 is an ugly hack) |
There was a problem hiding this comment.
I guess I'm missing something. What makes that an ugly hack?
There was a problem hiding this comment.
The error checking is completely disables if not replacement variables are specified in the input. The error checks should tell the user if they forgot to specify any variables in the input. If any variable is specified and the error check is triggered, the user needs to supply all variables for all models in the XML, not just the one that is selected.
All in all, that drove me to start working on Argonne-National-Laboratory/neml#100 which does checks and substitutions only on the XML subtree that represents the selected model.
There was a problem hiding this comment.
OK, thanks! Would you mind saying something to that effect in the comment?
Maybe related to this, I just discovered that if you point to a "model" that doesn't exist in the supplied xml file, you get a segfault in NEML.
There was a problem hiding this comment.
Also, the commit that adds this needs more of an explanation than "ugly hack" in the title. That doesn't make me feel very good from an SQA point of view... Maybe just squash your commits.
There was a problem hiding this comment.
if you point to a "model" that doesn't exist in the supplied xml file, you get a segfault in NEML
That's an easy fix I can add to my NEML PR
