feat: support non-scalar valued field specifications

[kdrienCG]

This PR generalizes field specifications to support non-scalar values, notably scales (and so function names).

The following interface is proposed:

<FieldSpecification
   name="perm_wall"
   initialCondition="1"
   setNames="{ wall1, wall2 }"
   objectPath="ElementRegions/Region1/block1"
   fieldName="rockPerm_permeability"
   scale="{ 2.0e-22, 2.0e-22, 2.0e-22 }"
/>

Instead of:

<FieldSpecification
   name="permx_wall"
   component="0"
   initialCondition="1"
   setNames="{ wall1, wall2 }"
   objectPath="ElementRegions/Region1/block1"
   fieldName="rockPerm_permeability"
   scale="2.0e-22"
/>

<FieldSpecification
   name="permy_wall"
   component="1"
   initialCondition="1"
   setNames="{ wall1, wall2 }"
   objectPath="ElementRegions/Region1/block1"
   fieldName="rockPerm_permeability"
   scale="2.0e-22"
/>

<FieldSpecification
   name="permz_wall"
   component="2"
   initialCondition="1"
   setNames="{ wall1, wall2 }"
   objectPath="ElementRegions/Region1/block1"
   fieldName="rockPerm_permeability"
   scale="2.0e-22"
/>

This PR also add a regionNames array to make it easier to specify multiple regions.

Considerations

Implementations considerations:

Design choice

I added a separate scales attribute (on top of the already existing scale) and a functionNames attribute (on top of the already existing functionName) to keep backward compatibility with every input files.

The user must choose one way to describe its field specification,
either the scalar way:

<FieldSpecification
   ...
   functionName="func"
   scale="11"
   ...
/>

or the non-scalar way:

<FieldSpecification
   ...
   functionNames="{ func1, func2, func3 }"
   scales="{ 11, 12, 13 }"
   ...
/>

and will not be able to use both at the same time.

Other possible implementations

There are different ways this PR could have been implemented, like:

• completly removing the scalar scale attribute in favor of a non-scalar scales
  • _{It would requires a massive refactor and break every input files (if no solution is found to treat scale="42" and scale="{ 42 }" as the same type)}
• use the blueprint/factory design (from feat: add PermeabilitySpecification as an high-level FieldSpecification #4019) to describe a new non-scalar component that creates multiple FieldSpecification
  • _{It would produce a lot of code duplication to have the same capabilities between all the new components (condensing multiple FieldSpecification in one, having a user-friendly way to specify a region, have similar validations etc.). Where those "high-level"/specialized components could be thin wrappers over this PR for custom validation, documentation, etc.}
• others.

[herve-gross]

@rrsettgast does this work for you?

[rrsettgast]

Really only two potential issues I have:

1. It seems like the tensorial specifications are a completely separate thing from the scalar specifications. They should be able to be made into the same thing.
2. The introduction of regionNames seems like it is duplicating the objectPath capability but just cutting out the ElementRegions/regionName*? Can you comment on how the usage is improved?

[kdrienCG]

Really only two potential issues I have:

1. It seems like the tensorial specifications are a completely separate thing from the scalar specifications. They should be able to be made into the same thing.
2. The introduction of regionNames seems like it is duplicating the objectPath capability but just cutting out the ElementRegions/regionName*? Can you comment on how the usage is improved?

@rrsettgast thanks for the review.

1. You are right that they should be one and the same (they were originally added to not break backward compatibility with user inputs, but I found another solution for this):
   I removed scales and functionNames and generalized scale and functionName directly. scale is now an array1d.
   I also modified how 1d arrays are parsed to accept single values written as "1.0" instead of only accepting "{ 1.0 }". This way it will not break every user input files.
   This behavior is propagated to every attributes using 1d arrays (like setNames, etc.) making them shorter to write when using single values.

2. For the regionNames, like you said it does not bring any capabilities beyond what objectPath already exposes.
   The motivation is purely ergonomic, making it shorter and more readable for users. I agree it is a bit less clean when subregions are involved because the / separator reappears (e.g. regionNames="{ region1/subregion1, region1/subregion2 }"), but it still avoids the ElementRegions/ boilerplate.
   regionNames also benefits from the behavior described above making it even simpler for single region: regionNames="region1"

[rrsettgast]

1. You are right that they should be one and the same (they were originally added to not break backward compatibility with user inputs, but I found another solution for this):
   I removed scales and functionNames and generalized scale and functionName directly. scale is now an array1d.
   I also modified how 1d arrays are parsed to accept single values written as "1.0" instead of only accepting "{ 1.0 }". This way it will not break every user input files.
   This behavior is propagated to every attributes using 1d arrays (like setNames, etc.) making them shorter to write when using single values.

This is great. However, I am not opposed to forcing an xml change to "{1.0}". I go back and forth on this. Avoiding the input file churn should not be a deciding factor in this decision. What do the users think?

2. For the regionNames, like you said it does not bring any capabilities beyond what objectPath already exposes.
   The motivation is purely ergonomic, making it shorter and more readable for users. I agree it is a bit less clean when subregions are involved because the / separator reappears (e.g. regionNames="{ region1/subregion1, region1/subregion2 }"), but it still avoids the ElementRegions/ boilerplate.
   regionNames also benefits from the behavior described above making it even simpler for single region: regionNames="region1"

Call this my anti-perl tendency, but I prefer a single way to do something rather than multiple ways. With the introduction of "regionNames" there are two ways to specify an element region, and multiple ways to be inconsistent. For instance regionNames and objectPath both being specified requires specific input error checks?

I do acknowledge that there was an intent to make objectPath better at some point. I do think it is possible to remove the requirement for "ElementRegions/" and just parse the remaining object path assuming it is there, unless the other objects are specified (i.e. nodeManager, edgeManager, faceManager). For instance:

in MeshObjecPath.cpp we have this:

MeshObjectPath::ObjectTypes extractObjectType( string const & path )
{
  MeshObjectPath::ObjectTypes objectType = MeshObjectPath::ObjectTypes::invalid;
  if( path.find( MeshLevel::groupStructKeys::nodeManagerString() ) != std::string::npos )
  {
    objectType = MeshObjectPath::ObjectTypes::nodes;
  }
  else if( path.find( MeshLevel::groupStructKeys::edgeManagerString() ) != std::string::npos )
  {
    objectType = MeshObjectPath::ObjectTypes::edges;
  }
  else if( path.find( MeshLevel::groupStructKeys::faceManagerString() ) != std::string::npos )
  {
    objectType = MeshObjectPath::ObjectTypes::faces;
  }
  else if( path.find( MeshLevel::groupStructKeys::elemManagerString() ) != std::string::npos )
  {
    objectType = MeshObjectPath::ObjectTypes::elems;
  }
  return objectType;
}

So it is always one of these values. (note: I think there should be an error case here)

If we changed the last else if to this:

  else
  {
    objectType = MeshObjectPath::ObjectTypes::elems;
  }
}

then it defaults to elements. I still prefer keeping "ElementRegions" as a requirement, but I don't think defaulting to it is horrible in theory...except for the fact?? that you cannot have an error check here.

[dkachuma]

Suggested change

	string_array m_functionName;
	string_array m_functionNames;

[dkachuma]

Suggested change

	constexpr static char const * functionNamesString() { return "functionNames"; }

[dkachuma]

Same for scale -> scales.

[kdrienCG]

To avoid breaking users' input files, I kept it as "scale" (same for "functionName"). However, if the previously mentioned change/commit to accept unbracketed single values for 1D arrays is rejected, I will rename it to scales (unless breaking users' files in this case is acceptable?).

[rrsettgast]

Do both. Enforce scales = {}

[dkachuma]

This isn't quite what I expected. I expected a loop over the region names. Has this been tested with multiple regions?

[kdrienCG]

Yes, regionNames works with multiple regions (tested with isothermalLeakyWell_smoke_3d.xml). It relies on an objectPath capability that accepts multiple "nodes" at the same level (e.g., objectPath="ElementRegions/{ region1 region2 }").

However, after testing, I found that using objectPath="ElementRegions/{ region1 region2 }" in a XML does not work, whereas regionNames="{ region1, region2 }" does. This is likely due to a regex in the parsing phase that blocks it.

[rrsettgast]

However, after testing, I found that using objectPath="ElementRegions/{ region1 region2 }" in a XML does not work, whereas regionNames="{ region1, region2 }" does. This is likely due to a regex in the parsing phase that blocks it.

This is not regex, it is fnmatch. This not a valid fnmatch pattern. Much to your users displeasure I think the valid fnmatch pattern is:

regionNames="ElementRegions/region[12]"

or full expansion:
regionNames="{ ElementRegions/region1, ElementRegions/region2 }"

annoyingly it would be hard (not possible) to get a fnmatch expansion to something like

ElementRegions/reservoir, ElementRegions/overburden

that would have to be:

{ ElementRegions/reservoir, ElementRegions/overburden }

alternatively if you take my suggestion that we just assume everything not FaceManager, EdgeManager, NodeManager is ElementRegion, then I think

objectPath="{ reservoir, overburden }"

would work as desired.

[kdrienCG]

I've removed the regionNames attribute, I'll open a separate PR for the objectPath