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

fix: Add HU simplified 2-phase version, refactor IHU implementation and fix bugs where possible, change phase/component flux logic #3401

Merged
merged 118 commits into from
Mar 5, 2025

Conversation

paveltomin
Copy link
Contributor

@paveltomin paveltomin commented Oct 16, 2024

  1. Another implementation of for hybrid upwinding, limited to 2 phases, tries to do total mobility treatment closer to published schemes
  2. Refactored IHU implementation, added few helper functions to minimize code duplication, fixed/attempted to fix the following bugs:
  • densMean * dGravD_dP should be outside of for( localIndex j = 0; j < numFluxSupportPoints; ++j ) subloop (not very important since dTrans_dPres is often zero), similar fix in capillary flux
  • totMob += phaseMob[seri[ke]][sesri[ke]][sei[ke]][jp] and derivatives should not be in the for( localIndex ke = 0; ke < numFluxSupportPoints; ++ke ) subloop - but i could not change this one without getting bad convergence for barrier test case - results with wrong treatment are definitely off but i am not sure what to do with that, @jafranc any thoughts?
  • derivatives for total mobility here: dFractionalFlow_dP[ke] -= fractionalFlow * dTotMob_dP[k_up_ppu] / LvArray::math::max( totMob, minTotMob ) should be taken from ke not k_up_ppu (same in other similar places)
  1. Change phase/component flux logic - assemble full phase flux first and then do x_cp upwinding once to compute component flux. The danger of old treatment is that phaseFlux being used in subsequent computations and it might be tricky to separate parts of phase flux to compute component flux

@paveltomin paveltomin removed the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Jan 15, 2025
@paveltomin paveltomin added ci: run integrated tests Allows to run the integrated tests in GEOS CI and removed ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Jan 17, 2025
@paveltomin paveltomin closed this Feb 7, 2025
@paveltomin paveltomin reopened this Feb 7, 2025
@paveltomin paveltomin requested a review from tjb-ltk as a code owner February 7, 2025 22:18
Copy link
Contributor

@tjb-ltk tjb-ltk left a comment

Choose a reason for hiding this comment

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

Nice refactoring to cleanup the phase and component flux calcs... clarity and avoiding redundant code certainly helps the next developer

@paveltomin
Copy link
Contributor Author

@CusiniM, @castelletto1, @frankfeifan, @jhuang2601, @rrsettgast please review

@paveltomin paveltomin removed the request for review from ryar9534 March 4, 2025 15:32
@paveltomin paveltomin requested a review from Guotong-Ren as a code owner March 4, 2025 15:32
@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 Mar 5, 2025
@paveltomin paveltomin merged commit 5fe125b into develop Mar 5, 2025
24 of 25 checks passed
@paveltomin paveltomin deleted the pt/hu-2phase branch March 5, 2025 23:42
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: requires rebaseline Requires rebaseline branch in integratedTests type: bug Something isn't working type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants