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.

Sign up for GitHub

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

Jump to bottom

Fixes to the special case with almost only water #803

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fixes to the special case with almost only water #803

wants to merge 1 commit into from

Conversation

totto82
Copy link
Member

@totto82 totto82 commented Apr 26, 2023

For the almost only water case the primary variables are p, sg and sw.

  1. Use sw>=1-1e-6 as threshold for defining almost only water
  2. Add sg to sw if sg<0 to make the oil saturation consistent even though sg is set to zero
  3. Chop the water saturation at 1.01 and not 1.0 in order to improve convergence
  4. Restrict update of rs/rv and rsw/rvw by the saturation scaling factor from the Appleyard-chopping.

@totto82
Copy link
Member Author

totto82 commented Apr 26, 2023

jenkins build this please

@totto82
Copy link
Member Author

totto82 commented Apr 26, 2023

benchmark please

@ytelses
Copy link

ytelses commented Apr 26, 2023

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.829
opm-git OPM Benchmark: drogon - Threads: 8 1.312
opm-git OPM Benchmark: smeaheia - Threads: 1 1.001
opm-git OPM Benchmark: smeaheia - Threads: 8 0.955
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 0.994
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 0.997
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 0.998
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 1.009
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.999
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.062
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2058

@totto82
Copy link
Member Author

totto82 commented May 16, 2023

jenkins build this please

@totto82 totto82 marked this pull request as ready for review May 22, 2023 09:25
@totto82
Copy link
Member Author

totto82 commented May 22, 2023

jenkins build this please

@totto82 totto82 requested a review from atgeirr May 22, 2023 09:26
@totto82
Copy link
Member Author

totto82 commented May 23, 2023

I have looked the test failures. At-least the following 3 test failures needs to be addressed as they dont finish
MIN_GASRATE_1
SPE5CASE1_DYN
compareParallelSim_flow+AQUFLUX-01

Fixes to the special case with almost only water
36a0976 
For the almost only water case the primary variables are p, sg and sw.

1) Use sw>=1-1e-6 as threshold for defining almost only water
2) Chop the water saturation at 1.01 and not 1.0 in order to improve convergence
3) Restrict update of rs/rv, rsw/rvw and zfraction in the extended blackoil model by the saturation scaling factor from the Appleyard-chopping.
@totto82
Copy link
Member Author

totto82 commented May 24, 2023

jenkins build this please

@totto82 totto82 mentioned this pull request May 24, 2023
Closed
@totto82
Copy link
Member Author

totto82 commented May 24, 2023

jenkins build this opm-simulators=4666 please

@totto82
Copy link
Member Author

totto82 commented May 25, 2023

benchmark please

@ytelses
Copy link

ytelses commented May 25, 2023

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.082
opm-git OPM Benchmark: drogon - Threads: 8 1.26
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.003
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.01
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 0.992
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 1.113
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.986
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.096
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2096

@alfbr
Copy link
Member

alfbr commented May 25, 2023

For what it's worth, the performance improvements here are very impressive. Seems like we have a candidate for improved numerics. I am ready to test this more broadly at our end, what do you think?

@totto82
Copy link
Member Author

totto82 commented May 25, 2023

I am ready to test this more broadly at our end, what do you think?

Yes, that would be nice. These changes will need some broad testing before it can be merged.

@totto82
Copy link
Member Author

totto82 commented Jun 2, 2023

I will split this PR in order to test the individual parts.

@totto82 totto82 marked this pull request as draft June 2, 2023 11:51
@totto82 totto82 mentioned this pull request Jun 2, 2023
Open
@alfbr
Copy link
Member

alfbr commented Jul 13, 2023

Finally got to test this extensively. The results are very promising, but I do have a couple of models with convergence issues. Give me a hint when we can pick up this one again.

This was referenced Aug 7, 2023
Merged
Closed
@alfbr
Copy link
Member

alfbr commented Aug 14, 2023

I am now happy with the changes here. They enable convergence on select cases that were unable to run in Flow before. The impact during ensemble testing are also positive in the sense that we do not see deterioration on cases where Flow runs fine today. From our end, the changes here are ready for merging.

atgeirr
atgeirr reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@atgeirr atgeirr left a comment

Choose a reason for hiding this comment

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

A few comments, I am a bit hesitant to merge this as-is.

opm/models/blackoil/blackoilprimaryvariables.hh
@@ -548,25 +546,30 @@ public:

// special case cells with almost only water
// use both saturations (if the phase is enabled)
// to avoid a singular matrix.
// almost is defined as sw >= 1.0-1e-6.
static const Scalar thresholdWaterFilledCell = 1.0 - 1e-6;
Copy link
Member

Choose a reason for hiding this comment

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

(Still in Draft, but I will make a few comments.)

Why not use the eps argument as before? Do we need to use eps 0 for other reasons?

opm/models/blackoil/blackoilprimaryvariables.hh
if constexpr (waterEnabled) {
(*this)[Indices::waterSwitchIdx] = 1.0;
(*this)[Indices::waterSwitchIdx] = std::min(1.01, sw);
Copy link
Member

Choose a reason for hiding this comment

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

It is very unfortunate if we have to let saturations go outside physical constraints, especially by so much. Is this change crucial? If so, could 1 + eps be used here instead of 1.01?

@totto82
Copy link
Member Author

totto82 commented Aug 14, 2023

I am now happy with the changes here. They enable convergence on select cases that were unable to run in Flow before. The impact during ensemble testing are also positive in the sense that we do not see deterioration on cases where Flow runs fine today. From our end, the changes here are ready for merging.

Thanks for the testing. I think the two first part i.e. PR #809 and #816 is ready to go inn. This does not change the physical constraint of the model buts adds it as a extra parameter. For #817 I am still doing some testing to understand the test failures.

@atgeirr This PR is for testing. Please review #809, #816 and #817 instead.

@atgeirr
Copy link
Member

atgeirr commented Aug 15, 2023

@atgeirr This PR is for testing. Please review #809, #816 and #817 instead.

Right! I got confused there, thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atgeirr atgeirr atgeirr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@totto82 @ytelses @alfbr @atgeirr