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: try this density treatment for gravity #3337

Merged
merged 24 commits into from
Nov 7, 2024
Merged

Conversation

paveltomin
Copy link
Contributor

@paveltomin paveltomin commented Sep 6, 2024

currently density in gravity term is 0.5(rho_i+rho_j) where i and j are the cells
here we are changing it to more commonly used treatment that checks phase presence leading to:

  1. 0.5(rho_i+rho_j) when phase is present in both cells
  2. rho_i when only in i
  3. rho_j when only in j
    probably not a big deal but better to be consistent with mainstream way

@paveltomin paveltomin changed the title try this density treatment for gravity fix: try this density treatment for gravity Sep 6, 2024
@paveltomin paveltomin self-assigned this Sep 6, 2024
@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Sep 6, 2024
Copy link

codecov bot commented Sep 7, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 25 lines in your changes missing coverage. Please review.

Project coverage is 57.56%. Comparing base (77bab14) to head (48f2ef8).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...ermalCompositionalMultiphaseFVMKernelUtilities.hpp 50.00% 14 Missing ⚠️
...ls/IsothermalCompositionalMultiphaseFVMKernels.cpp 20.00% 8 Missing ⚠️
...ls/IsothermalCompositionalMultiphaseFVMKernels.hpp 33.33% 2 Missing ⚠️
...rnels/ThermalCompositionalMultiphaseFVMKernels.hpp 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3337      +/-   ##
===========================================
- Coverage    57.56%   57.56%   -0.01%     
===========================================
  Files         1092     1092              
  Lines        97800    97837      +37     
===========================================
+ Hits         56302    56320      +18     
- Misses       41498    41517      +19     

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

@paveltomin paveltomin added the flag: requires rebaseline Requires rebaseline branch in integratedTests label Sep 9, 2024
}
denom++;
}
if( denom > 1 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Could a phase potentially be absent in both cells? Or is this checked elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be absent in both yes, there is a check below but it would be better to check even outside of the function
Just don't want to add more complexity for this PR

for( localIndex i = 0; i < numFluxSupportPoints; ++i )
{
localIndex const er = seri[i];
localIndex const esr = sesri[i];
localIndex const ei = sei[i];

bool const phaseExists = (phaseVolFrac[er][esr][ei][ip] > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

In some cases I have seen saturation weighted density in this case. Not sure what the benefit of that is over a simpler mean. Also looks like it might complicate the derivatives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it should be more accurate, but I don't want to complicate the derivatives

@paveltomin paveltomin added flag: ready for review and removed ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Sep 18, 2024
@paveltomin paveltomin requested a review from jafranc September 20, 2024 22:04
@paveltomin
Copy link
Contributor Author

review please?

@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 type: bug Something isn't working and removed flag: ready for review labels Sep 27, 2024
@paveltomin paveltomin added the ci: run code coverage enables running of the code coverage CI jobs label Oct 4, 2024
@paveltomin
Copy link
Contributor Author

@CusiniM, @castelletto1, @cssherman, @jhuang2601, @rrsettgast, and/or @wrtobin could you please review?

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.

I feel it should be weighted with the saturation but this is better than before.

@CusiniM
Copy link
Collaborator

CusiniM commented Oct 14, 2024

I feel it should be weighted with the saturation but this is better than before.

or maybe using the mass fraction since it's a density.

@paveltomin
Copy link
Contributor Author

need to adjust thermal part too
move out from merge queue for now

@paveltomin paveltomin merged commit 61ef0e0 into develop Nov 7, 2024
25 of 26 checks passed
@paveltomin paveltomin deleted the bt/grav-dens branch November 7, 2024 14:48
@paveltomin paveltomin mentioned this pull request Nov 8, 2024
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: requires rebaseline Requires rebaseline branch in integratedTests type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants