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: make new gravity treatment from #3337 an option #3467

Merged
merged 8 commits into from
Dec 7, 2024

Conversation

paveltomin
Copy link
Contributor

disabled by default since SPE11 case degradations were observed

@paveltomin paveltomin added flag: ready for review ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Nov 27, 2024
@paveltomin paveltomin self-assigned this Nov 27, 2024
Copy link
Contributor

@victorapm victorapm left a comment

Choose a reason for hiding this comment

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

This PR matches with the reference results for spe11b. Thanks!

spe11b-times

@paveltomin
Copy link
Contributor Author

@CusiniM and @ryar9534 let's please move this fix quickly into develop, critical for spe11

@paveltomin paveltomin added flag: requires rebaseline Requires rebaseline branch in integratedTests and removed ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Dec 2, 2024
@CusiniM
Copy link
Collaborator

CusiniM commented Dec 6, 2024

I don' t know that we really need 2 options for the treatment of gravity. In general, I think that what @paveltomin had introduced made more sense... I am not too sure why it would have such a large impact on convergence.

@paveltomin paveltomin added type: feature New feature or request ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI ci: run code coverage enables running of the code coverage CI jobs and removed flag: ready for review labels Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 43.18182% with 50 lines in your changes missing coverage. Please review.

Project coverage is 57.11%. Comparing base (b56762c) to head (705fef5).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...vers/fluidFlow/kernels/compositional/CFLKernel.cpp 0.00% 28 Missing ⚠️
...s/fluidFlow/kernels/compositional/IHUPhaseFlux.hpp 0.00% 16 Missing ⚠️
...idFlow/kernels/compositional/FluxComputeKernel.hpp 50.00% 3 Missing ⚠️
...sSolvers/fluidFlow/CompositionalMultiphaseBase.cpp 85.71% 1 Missing ⚠️
...fluidFlow/kernels/compositional/C1PPUPhaseFlux.hpp 0.00% 1 Missing ⚠️
...olvers/fluidFlow/kernels/compositional/PotGrad.hpp 96.15% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3467      +/-   ##
===========================================
- Coverage    57.11%   57.11%   -0.01%     
===========================================
  Files         1151     1151              
  Lines        99454    99475      +21     
===========================================
+ Hits         56808    56818      +10     
- Misses       42646    42657      +11     

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

@paveltomin paveltomin merged commit be0eb0e into develop Dec 7, 2024
23 of 25 checks passed
@paveltomin paveltomin deleted the pt/new-grav-option branch December 7, 2024 03:03
@rrsettgast
Copy link
Member

@paveltomin The term newGravity is quite uninformative. Can you create descriptive options for the "gravity treatment" so that developers/users know what the treatments are?

@paveltomin
Copy link
Contributor Author

@paveltomin The term newGravity is quite uninformative. Can you create descriptive options for the "gravity treatment" so that developers/users know what the treatments are?

Ok will do

@paveltomin
Copy link
Contributor Author

@paveltomin The term newGravity is quite uninformative. Can you create descriptive options for the "gravity treatment" so that developers/users know what the treatments are?

is this #3486 better?

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: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants