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: Transmissibility output #3091

Merged
merged 86 commits into from
Aug 6, 2024
Merged

Conversation

MelReyCG
Copy link
Contributor

@MelReyCG MelReyCG commented Apr 24, 2024

This PR is after the #3247.


This PR aims to add a new component: the StencilOutput, that allow to add a CellToCellOutput task node in the xml to output ConnectionData (global id pairs, transmissibilities).

For now, by using the writeCSV="1" attribute, the CellToCellOutput will output this data in a CSV. Here is an exemple:

  • XML excerpt
  <Solvers gravityVector="{0.0, 0.0, 0.0}">
    <SinglePhaseFVM
      name="singlePhaseFlowSolver"
      […]
    </SinglePhaseFVM>
  </Solvers>

  <Tasks>
    <CellToCellOutput
      name="stencilOutput"
      flowSolverName="singlePhaseFlowSolver"
      writeCSV="1"
      logLevel="1" />
  </Tasks>

  <Events maxTime="10.0">
    <SoloEvent
      name="stencilOutputEvent"
      targetCycle="1"
      target="/Tasks/stencilOutput" />
  </Events>
  • First 10 lines of the generated HDF5
time [s] , A global id , B global id , AB transmissibility , BA transmissibility
       4 ,           0 ,        2715 , 7.55463e-18                          , -7.55463e-18                        
       4 ,           0 ,       47546 , 8.06167e-18                          , -8.06167e-18                        
       4 ,           0 ,      134459 , 6.63826e-18                          , -6.63826e-18                        
       4 ,           0 ,      165202 , 7.78139e-18                          , -7.78139e-18                        
       4 ,           1 ,        4642 , 1.83325e-17                          , -1.83325e-17                        
       4 ,           1 ,       53907 , 1.76737e-17                          , -1.76737e-17                        
       4 ,           1 ,      184341 , 1.81705e-17                          , -1.81705e-17                        
       4 ,           2 ,        5459 , 1.7414e-17                           , -1.7414e-17                         
       4 ,           2 ,       63907 , 1.86145e-17                          , -1.86145e-17                        
       4 ,           2 ,      177257 , 1.86145e-17                          , -1.86145e-17                        

(aligned here for readability)

@MelReyCG MelReyCG self-assigned this Apr 24, 2024
@MelReyCG MelReyCG added the type: feature New feature or request label Apr 24, 2024
@MelReyCG MelReyCG requested review from paveltomin and ryar9534 April 26, 2024 12:01
@paveltomin paveltomin requested a review from CusiniM April 26, 2024 13:33
outputFile << time << ","
<< conn.globalId[0] << ","
<< conn.globalId[1] << ","
<< conn.transmissibility[0] << ","
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure it makes much sense to print the same transmissibility value twice but it's ok, not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it will be more useful to output two half-transmissibilities.

FiniteVolumeManager const & fvManager = numericalMethodManager.getFiniteVolumeManager();
FluxApproximationBase const & fluxApprox = fvManager.getFluxApproximation( m_solver->getDiscretizationName() );

fluxApprox.forStencils< CellElementStencilTPFA >( mesh, [&]( auto const & stencil )
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please check if all that will work for https://github.com/GEOS-DEV/GEOS/blob/develop/inputFiles/compositionalMultiphaseFlow/co2_hybrid_1d.xml ?
not sure hybrid discretization has the same things as TPFA

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the key here is that fvManager.getFluxApproximation( m_solver->getDiscretizationName() ); might return null if the solver doesn't have a flux approximation. So I would add a validator (postProcessInput()) to the solverName field of this to ensure that the solver provided has the appropriate flux approximation.

FiniteVolumeManager const & fvManager = numericalMethodManager.getFiniteVolumeManager();
FluxApproximationBase const & fluxApprox = fvManager.getFluxApproximation( m_solver->getDiscretizationName() );

fluxApprox.forStencils< CellElementStencilTPFA >( mesh, [&]( auto const & stencil )
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the key here is that fvManager.getFluxApproximation( m_solver->getDiscretizationName() ); might return null if the solver doesn't have a flux approximation. So I would add a validator (postProcessInput()) to the solverName field of this to ensure that the solver provided has the appropriate flux approximation.

Copy link
Collaborator

@CusiniM CusiniM left a comment

Choose a reason for hiding this comment

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

It seems that we are trying to only write a file for rank 0 but I don't see when you gather all stencil data there? This would only work for small cases....
In any case, why not using the timehistory and writing in a parallel format instead? @wrtobin opinions?

Besides debugging I don't really see why someone would want to write out transmissibilities.

MelReyCG added 4 commits June 13, 2024 10:08
Point is to allow to do an HDF output, and remove the CSV one.
…end.

Point is to solve a bug where the StencilDataCollection buffer size are not known for the HDF chunk sizes.
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 76.37795% with 60 lines in your changes missing coverage. Please review.

Project coverage is 56.00%. Comparing base (0cbc589) to head (9c93b4d).
Report is 75 commits behind head on develop.

Files with missing lines Patch % Lines
src/coreComponents/common/Units.hpp 14.58% 41 Missing ⚠️
...physicsSolvers/fluidFlow/StencilDataCollection.cpp 87.27% 14 Missing ⚠️
src/coreComponents/common/logger/Logger.hpp 0.00% 3 Missing ⚠️
...vers/multiphysics/CoupledReservoirAndWellsBase.hpp 0.00% 1 Missing ⚠️
.../unitTests/fluidFlowTests/testTransmissibility.cpp 98.48% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3091      +/-   ##
===========================================
+ Coverage    55.91%   56.00%   +0.09%     
===========================================
  Files         1043     1046       +3     
  Lines        88934    89122     +188     
===========================================
+ Hits         49729    49915     +186     
- Misses       39205    39207       +2     

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

@MelReyCG MelReyCG added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Jul 29, 2024
@MelReyCG MelReyCG added the flag: no rebaseline Does not require rebaseline label Aug 1, 2024
@MelReyCG
Copy link
Contributor Author

MelReyCG commented Aug 2, 2024

Just leaving a note here, no CI step is legitimately failing here. The CUDA builds were working but crash now because of a CI bug since I updated my branch.

@CusiniM
Copy link
Collaborator

CusiniM commented Aug 2, 2024

This PR needs one more review.

@rrsettgast rrsettgast merged commit 6972791 into develop Aug 6, 2024
22 checks passed
@rrsettgast rrsettgast deleted the feature/rey/transmissibilityOutput branch August 6, 2024 02:33
rrsettgast pushed a commit that referenced this pull request Sep 17, 2024
* added transmissibility unit

* Added Cell-To-Cell output

* Add the logLevel viewkey & description (cherrypick from feature/rey/sourceflux-stats)

* Refactor the StencilOutput so the PackCollection can read it.
Point is to allow to do an HDF output, and remove the CSV one.

* Changing the Problem sub-groups init order so the outputs are at the end.
Point is to solve a bug where the StencilDataCollection buffer size are not known for the HDF chunk sizes.

* freeOnDevice fix test
rrsettgast pushed a commit that referenced this pull request Sep 17, 2024
* added transmissibility unit

* Added Cell-To-Cell output

* Add the logLevel viewkey & description (cherrypick from feature/rey/sourceflux-stats)

* Refactor the StencilOutput so the PackCollection can read it.
Point is to allow to do an HDF output, and remove the CSV one.

* Changing the Problem sub-groups init order so the outputs are at the end.
Point is to solve a bug where the StencilDataCollection buffer size are not known for the HDF chunk sizes.

* freeOnDevice fix test
@MelReyCG MelReyCG linked an issue Oct 21, 2024 that may be closed by this pull request
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 type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a cell / cell transmissibility data table
7 participants