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

refactor: Factoring hysteresis model out of TableRelativePermeabilityHysteresis #2207

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

Conversation

jafranc
Copy link
Contributor

@jafranc jafranc commented Dec 9, 2022

Refactor TableRelativePermeabilityHysteresis in preparation for #2159.
A KilloughHysteresis class will be created taking responsabilities for:

  • compute portion of trapped phase
  • store and provide access to Killough model parameters (jerauld coeff, curvature coeff, reversal buffers)
  • define HystersisCurve type storing major point of Hysteresis curve in a fashion of {sat,value} pair for all three points.

The KilloughHysteresis could also be responsible for computing Land Coefficient and all imbibition values as it is specific to the model how to compute these intermediate scanning curves. It can also be specialized further down to have a KilloughHysteresisRelativePermeability and KilloughHysteresisCapillaryPressure.

Eventually if new model improving capillary pressure treatment ( e.g Kleppe model [2]), it can be turn into a more templatized design.

Additionally,

  • revise unitTest for TableRelativePermeabilityHysteresis

Rebaseline done in GEOS-DEV/integratedTests#24

jafranc and others added 4 commits December 10, 2022 04:26
# Conflicts:
#	src/coreComponents/constitutive/CMakeLists.txt
#	src/coreComponents/constitutive/capillaryPressure/CapillaryPressureBase.cpp
#	src/coreComponents/constitutive/capillaryPressure/CapillaryPressureExtrinsicData.hpp
#	src/coreComponents/constitutive/relativePermeability/RelativePermeabilityExtrinsicData.hpp
#	src/coreComponents/constitutive/relativePermeability/TableRelativePermeabilityHysteresis.cpp
#	src/coreComponents/constitutive/thermalConductivity/MultiPhaseThermalConductivityFields.hpp
#	src/coreComponents/constitutive/thermalConductivity/SinglePhaseThermalConductivityFields.hpp
#	src/coreComponents/constitutive/thermalConductivity/ThermalConductivityFields.hpp
#	src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseBase.cpp
@jafranc jafranc changed the title Jafranc/refactor/relperm hysteresis Factoring hysteris model out of TableRelativePermeabilityHysteresis Dec 13, 2022
@jafranc
Copy link
Contributor Author

jafranc commented Oct 12, 2023

We face some GPU issue IIRC, let us fix that soon

@paveltomin
Copy link
Contributor

We face some GPU issue IIRC, let us fix that soon

any update?

@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 and removed flag: ready for review labels Dec 14, 2023
@paveltomin paveltomin force-pushed the jafranc/refactor/relpermHysteresis branch from b9e1ae5 to fdbfbd1 Compare December 14, 2023 15:47
@rrsettgast
Copy link
Member

@jafranc Can you merge develop into this and one of us will tackle the GPU compilation issues?

…esis

# Conflicts:
#	integratedTests
#	src/coreComponents/constitutive/CMakeLists.txt
#	src/coreComponents/constitutive/relativePermeability/TableRelativePermeabilityHysteresis.cpp
#	src/coreComponents/constitutive/relativePermeability/TableRelativePermeabilityHysteresis.hpp
#	src/coreComponents/constitutive/thermalConductivity/SinglePhaseThermalConductivityFields.hpp
#	src/coreComponents/constitutiveDrivers/relativePermeability/RelpermDriverRunTest.hpp
#	src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseBase.hpp
#	src/coreComponents/physicsSolvers/fluidFlow/IsothermalCompositionalMultiphaseFVMKernelUtilities.hpp
#	src/coreComponents/physicsSolvers/fluidFlow/wells/CompositionalMultiphaseWell.hpp
#	src/coreComponents/physicsSolvers/solidMechanics/SolidMechanicsLagrangianFEM.cpp
#	src/coreComponents/schema/docs/TableRelativePermeabilityHysteresis.rst
#	src/coreComponents/schema/docs/TableRelativePermeabilityHysteresis_other.rst
#	src/coreComponents/schema/schema.xsd.other
#	src/coreComponents/unitTests/constitutiveTests/CMakeLists.txt
@jafranc jafranc changed the title Factoring hysteresis model out of TableRelativePermeabilityHysteresis refactor: Factoring hysteresis model out of TableRelativePermeabilityHysteresis Dec 18, 2024
@jafranc jafranc added flag: ready for review ci: run code coverage enables running of the code coverage CI jobs labels Dec 20, 2024
@paveltomin
Copy link
Contributor

@jafranc seems to build ok, is it good to merge?

geos::real64 & phaseMax, geos::real64 & phaseMin )
{

TableCapillaryPressureHelpers::validateCapillaryPressureTable( capPresTable, fullConstitutiveName, capPresMustBeIncreasing );
Copy link
Contributor

Choose a reason for hiding this comment

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

what is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC it is just refactoring but could easily be reverted to develop

Copy link
Contributor

Choose a reason for hiding this comment

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

seems the function calls itself recursively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the same signature so different function no ?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh man, this is high class :)

return m_isWetting;
}

// /**
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right :)

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 73.63014% with 77 lines in your changes missing coverage. Please review.

Project coverage is 56.74%. Comparing base (93f0252) to head (34efee4).
Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
...rmeability/TableRelativePermeabilityHysteresis.hpp 72.72% 15 Missing ⚠️
...apillaryPressure/TableCapillaryPressureHelpers.cpp 0.00% 11 Missing ⚠️
...vers/relativePermeability/RelpermDriverRunTest.hpp 0.00% 8 Missing ⚠️
...meability/BrooksCoreyBakerRelativePermeability.hpp 0.00% 6 Missing ⚠️
...vePermeability/BrooksCoreyRelativePermeability.hpp 0.00% 6 Missing ⚠️
...eability/BrooksCoreyStone2RelativePermeability.hpp 0.00% 6 Missing ⚠️
...relativePermeability/TableRelativePermeability.hpp 0.00% 6 Missing ⚠️
...eability/VanGenuchtenBakerRelativePermeability.hpp 0.00% 6 Missing ⚠️
...ability/VanGenuchtenStone2RelativePermeability.hpp 0.00% 6 Missing ⚠️
...tiveDrivers/relativePermeability/RelpermDriver.cpp 0.00% 4 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2207      +/-   ##
===========================================
- Coverage    56.82%   56.74%   -0.08%     
===========================================
  Files         1154     1156       +2     
  Lines        99984    99950      -34     
===========================================
- Hits         56816    56720      -96     
- Misses       43168    43230      +62     

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

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: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants