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

feat: Simplification & consolidation of CellElementRegion definition #2878

Merged
merged 146 commits into from
Oct 17, 2024

Conversation

MelReyCG
Copy link
Contributor

@MelReyCG MelReyCG commented Dec 8, 2023

This PR comes after the #3247.


In order to simplify the cellBlocks referencing and thus simplify user workflows, this PR propose the following functionalities:


  1. Possibility to reference all cellBlock from a regionAttribute at once, without listing all primitives:
    <CellElementRegion
      name="burdens"
      cellBlocks="{ 1, 4, 5 }"
      materialList="{}" />

instead of:

    <CellElementRegion
      name="burdens"
      cellBlocks="{ 1_tetrahedra, 1_pyramids, 1_hexahedra, ...
                    4_tetrahedra, 4_pyramids, 4_hexahedra, ...
                    5_tetrahedra, 5_pyramids, 5_hexahedra, ... }"
      materialList="{}" />

  1. Possibility to reference all cellBlocks with fnmatch patterns:
    <CellElementRegion
      name="reservoir"
      cellBlocks="{ * }"
      materialList="{}" />
    <CellElementRegion
      name="reservoir"
      cellBlocks="{ [1-4]_* }"
      materialList="{}" />

  1. This PR also improved the cellBlock errors:
  • Improved the error when a cellBlock is referenced twice
  • Added an error when the cellBlockAttributeValues, cellBlocks, cellBlocksMatch attributes are mixed (allowing only one way to select cellblocks).
  • Improved the error when a referenced cellBlock is missing,
  • Improved the error when failing to select cellBlocks.
    See the form and more details here: new cellBlocks errors.pdf (not up to date).

This PR also:

  • Updates the Sphinx documentation page as it was incomplete, by providing the whole necessary python package list and a part to describe how to generate the .rst class tables.
  • Adds some .rst tables that were not previously pushed (forgotten?).

- Error when a cellBlock is referenced twice
- Proper error when a referenced cellBlock is missing
- Proper error when a cellBlock is not referenced
- Possibility to reference all cellBlocks with "all"
- Possitibility to reference all cellBlock from a region ("1" instead of "1_tetrahedra, 1_pyramids...")
@MelReyCG MelReyCG added the type: feature New feature or request label Dec 8, 2023
@MelReyCG MelReyCG self-assigned this Dec 8, 2023
@MelReyCG MelReyCG linked an issue Dec 22, 2023 that may be closed by this pull request
@MelReyCG MelReyCG marked this pull request as ready for review March 8, 2024 13:05
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 92.43243% with 14 lines in your changes missing coverage. Please review.

Project coverage is 56.79%. Comparing base (263e5fd) to head (410b9d5).
Report is 71 commits behind head on develop.

Files with missing lines Patch % Lines
...ponents/unitTests/meshTests/testElementRegions.cpp 73.68% 10 Missing ⚠️
src/coreComponents/mesh/ElementType.hpp 82.35% 3 Missing ⚠️
...c/coreComponents/common/format/StringUtilities.hpp 92.30% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2878      +/-   ##
===========================================
+ Coverage    56.47%   56.79%   +0.31%     
===========================================
  Files         1074     1076       +2     
  Lines        90504    95743    +5239     
===========================================
+ Hits         51116    54377    +3261     
- Misses       39388    41366    +1978     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@TotoGaz TotoGaz left a comment

Choose a reason for hiding this comment

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

Good that you can start working on this.
You may be interested in #1963 where we discuss how we can select objects in geos.

@MelReyCG
Copy link
Contributor Author

MelReyCG commented Oct 2, 2024

@rrsettgast
Following your suggestion, I merged the 3 cellBlocks, cellBlocksMatch, cellBlockAttributeValues attributes behaviour in the original cellBlocks one. It is now able to take all types of inputs (exact names, fnmatch patterns, region attribute).
Here are some supported exemples:

      <CellElementRegion name="everything" materialList="{ }" cellBlocks="{ * }" />
      <CellElementRegion name="overburden" materialList="{ }" cellBlocks="{ 1, 5 }" />
      <CellElementRegion name="reservoir" materialList="{ }" cellBlocks="{ 2, 6 }" />
      <CellElementRegion name="mixed" materialList="{ }"
                         cellBlocks="{ 1, 2_hexahedra, 2_tetrahedra, 2_pyramids, [5-6]_* }" />
      <CellElementRegion name="legacy" materialList="{ }"
                         cellBlocks="{ 1_hexahedra, 1_tetrahedra, 1_pyramids, 5_hexahedra, 5_tetrahedra, 5_pyramids }" />

@MelReyCG MelReyCG added the ci: run code coverage enables running of the code coverage CI jobs label Oct 3, 2024
@paveltomin
Copy link
Contributor

paveltomin commented Oct 7, 2024

I have not planned it yet, but I would be interested by what you have in mind on how they could be simplified.

how about this situation:

<Solvers gravityVector="{0.0, 0.0, -9.81}">
<MultiphasePoromechanicsReservoir name="poromechanicsReservoirSolver" poromechanicsSolverName="poromechanicsSolver" wellSolverName="wellSolver" initialDt="1e6" logLevel="1" targetRegions="{ Domain, WELL.W001, WELL.W002, WELL.W003, WELL.W004, WELL.W005 }">

it would be nice to be able to use patterns like that:

<Solvers gravityVector="{0.0, 0.0, -9.81}">
<MultiphasePoromechanicsReservoir name="poromechanicsReservoirSolver" poromechanicsSolverName="poromechanicsSolver" wellSolverName="wellSolver" initialDt="1e6" logLevel="1" targetRegions="{ Domain, WELL.* }">

i am sure users would appreciate

@MelReyCG
Copy link
Contributor Author

MelReyCG commented Oct 7, 2024

Sure! We will add a next PR will propose this pattern feature to more fields.

@MelReyCG MelReyCG removed the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Oct 17, 2024
@MelReyCG
Copy link
Contributor Author

By using a modified version of restart_check.py that allowed me to display the text arrays and indicate that they were only reordered, I think I can safely say that we can rebaseline, and add this in BASELINE_NOTES.md:

PR #2878 (2024-10-17)
=====================
Sorted region cellBlocks names alphabetically. Therefore affected ordering of: faceManager/elemSubRegionList, nodeManager/elemList, nodeManager/elemSubRegionList, SurfaceElementSubRegion::fractureElementsToCellSubRegions, field::perforation::reservoirElementSubregion.

I also changed the tag in .integrated_tests.yaml. I would say that this PR should be now ready to be merged but it is the first time I do a rebaseline, so don't hesitate to tell me if I did not that properly.

I opened a PR to propose those changes to restart_check.py.

@paveltomin paveltomin added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Oct 17, 2024
@paveltomin paveltomin merged commit 7eee999 into develop Oct 17, 2024
25 checks passed
@paveltomin paveltomin deleted the feature/rey/cellblocks-easier-naming branch October 17, 2024 18:24
@MelReyCG
Copy link
Contributor Author

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run code coverage enables running of the code coverage CI jobs ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify definition of regions in the XML
7 participants