Skip to content

fix: FieldSpeciaficationManager log corrections #3749

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

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

arng40
Copy link
Contributor

@arng40 arng40 commented Aug 1, 2025

This PR aim to correct and update some log introduced with #3658

  • An incorrect log was output when we specify an objectpath="ElementRegions" with an invalid fieldName
    Example with one of our test deck
<FieldSpecification
      name="topInitialPressure"
      initialCondition="1"
      setNames="{ top }"
      objectPath="ElementRegions"
      fieldName="pressuree"
      scale="5e6"/>

Currently we have

***** LOCATION: /data/pau901/VDG_GSI/ARNAUDD/geosRepos/codes/GEOS5/src/coreComponents/fieldSpecification/FieldSpecificationManager.cpp:272
***** Controlling expression (should be false): true
***** Rank 0: 
initialPressure/fieldName (simulationA_prop_erreurP.xml, l.326): there are no fieldName named `pressuree` under the region `ElementRegions`.

With the correction

FieldSpecificationManager.cpp:293
***** Controlling expression (should be false): true
***** Rank 0: 
initialPressure/fieldName (simulationA_prop_erreurP.xml, l.326): there are no fieldName named `pressuree` under the region `ElementRegions`.
 ElementRegions contain the following fields : 
 { bcPressure, bcTemperature, compAmount, compAmount_n, componentCFLNumber, componentOutflux, dGlobalCompFraction_dGlobalCompDensity, dPhaseMobility, dPhaseVolumeFraction, deltaPressure, deltaVolume, 
 fluid_phaseCompFraction, fluid_phaseDensity, fluid_phaseFraction, 
 [...] 
rockPorosity_salt_referencePorosity, temperature, temperatureScalingFactor, temperature_n }
There are 5 also CellElementsRegions that can be appended under ElementRegions : Reservoir, Caprock, Overburden, Underburden, Salt.

  • When we specify a region with no materials :
<FieldSpecification
      name="topInitialPressure"
      initialCondition="1"
      setNames="{ top }"
      objectPath="ElementRegions/Barrier"
      fieldName="pressuree"
      scale="5e6"/>
      ```

Before

***** LOCATION: /data/pau901/VDG_GSI/ARNAUDD/geosRepos/codes/GEOS2/src/coreComponents/fieldSpecification/FieldSpecificationManager.cpp:238
***** Controlling expression (should be false): true
***** Rank 0: 
topInitialPressure/fieldName (deadoil_2ph_staircase_gravity_segregation_3d.xml, l.132): there is no fieldName named `pressure` under the objectPath `ElementRegions/Barrier`.
Available fields in ElementRegions/Barrier are:

Now we got this message

***** LOCATION: /data/pau901/VDG_GSI/ARNAUDD/geosRepos/codes/GEOS5/src/coreComponents/fieldSpecification/FieldSpecificationManager.cpp:293
***** Controlling expression (should be false): true
***** Rank 0: 
topInitialPressure/fieldName (deadoil_2ph_staircase_gravity_segregation_3d.xml, l.132): there are no fieldName named `pressure` under the region `ElementRegions/Barrier`.
No material found under ElementRegions/Barrier.

@arng40 arng40 changed the title Log modifications and corrections FieldSpeciaficationManager log corrections Aug 1, 2025
@@ -250,5 +250,19 @@ stdVector< STRING_T > wrapTextToMaxLength( stdVector< STRING_T > const & lines,
template stdVector< string > wrapTextToMaxLength( stdVector< string > const &, size_t & );
template stdVector< string_view > wrapTextToMaxLength( stdVector< string_view > const &, size_t & );

string_array splitStringWithDelim( string const & s, char const delim )
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there already a tokenize method to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

CONTAINER< string > tokenize( string const & str,

FieldSpecificationBase::viewKeyStruct::fieldNameString(),
fs.getFieldName(), fs.getObjectPath() );;

string_array const splitPath = stringutilities::splitStringWithDelim( fs.getObjectPath(), '/' );
Copy link
Contributor

@dkachuma dkachuma Aug 1, 2025

Choose a reason for hiding this comment

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

I'm not sure about these path manipulations. Feels like something that should be handled by dataRepository. Is it possible to use

template< typename REGIONTYPE = ElementRegionBase, typename ... REGIONTYPES, typename LOOKUP_CONTAINER, typename LAMBDA >
void ElementRegionManager::forElementRegions( LOOKUP_CONTAINER const & targetRegions, LAMBDA && lambda ) const

Or additionally/alternatively the subRegion version of this.

* @param delim The character used as the delimiter for splitting the string.
* @return A vector containing the substrings obtained from the input string.
*/
string_array splitStringWithDelim( string const & s, char const delim );
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONTAINER< string > tokenize( string const & str,

Copy link
Collaborator

@wrtobin wrtobin left a comment

Choose a reason for hiding this comment

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

prefer existing tokenize over a redundant implementation, other than that this is fine

@arng40 arng40 added the flag: no rebaseline Does not require rebaseline label Aug 4, 2025
@arng40 arng40 changed the title FieldSpeciaficationManager log corrections fix: FieldSpeciaficationManager log corrections Aug 4, 2025
@paveltomin paveltomin requested review from paveltomin and removed request for castelletto1, herve-gross, jhuang2601, OmarDuran and paveltomin August 5, 2025 17:19
@paveltomin
Copy link
Contributor

prefer existing tokenize over a redundant implementation, other than that this is fine

seems resolved

@paveltomin paveltomin added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline flag: ready for review type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants