opm-simulators icon indicating copy to clipboard operation
opm-simulators copied to clipboard

Clean up: remove code in if(false) branch

Open lisajulia opened this issue 10 months ago • 8 comments

This logic was added in commit https://github.com/OPM/opm-simulators/commit/035d2166414d97c7f45e5e42496c162cc20bccb4 (PR https://github.com/OPM/opm-simulators/pull/4895). @steink: Can you have a look at this?

lisajulia avatar Feb 19 '25 12:02 lisajulia

jenkins build this please

lisajulia avatar Feb 19 '25 12:02 lisajulia

I am fine with removing this if @steink agrees. But I have lately tested an approach that stops iterating (i.e. just give up) if it stagnates multiple times. This potentially gives some speedup but more testing is needed. Is is hard to add a communication step here to fix the parallel issue? You dont need to do it, I just want to know in case I pick this idea up at a later stage. And what about the similar code in iterateWellEqWithControl(...) does that create problems with MPI

totto82 avatar Feb 19 '25 12:02 totto82

I am fine with removing this if @steink agrees. But I have lately tested an approach that stops iterating (i.e. just give up) if it stagnates multiple times. This potentially gives some speedup but more testing is needed. Is is hard to add a communication step here to fix the parallel issue? You dont need to do it, I just want to know in case I pick this idea up at a later stage. And what about the similar code in iterateWellEqWithControl(...) does that create problems with MPI

We can also leave this code in here if it is needed for future purposes, also there is no communication needed here, I just noticed this when I was looking through MultisegmentWell_impl.hpp, but all good.

lisajulia avatar Feb 19 '25 14:02 lisajulia

I have to admit I don't recall this was part of https://github.com/OPM/opm-simulators/pull/4895, but I'm fine with deleting it. In general I'm sceptical to accepting wells with very loose convergence criteria because well convergence problems is typically a symptom of something else than just that the well is difficult to solve.

steink avatar Feb 19 '25 15:02 steink

I have to admit I don't recall this was part of #4895, but I'm fine with deleting it. In general I'm sceptical to accepting wells with very loose convergence criteria because well convergence problems is typically a symptom of something else than just that the well is difficult to solve.

@steink Thanks :) then this can be merged safely?

lisajulia avatar Feb 20 '25 09:02 lisajulia

I just added a draft PR that start using this part of the code https://github.com/OPM/opm-simulators/pull/6020 I think some code that detects stagnation / oscillation is a good idea.

totto82 avatar Feb 20 '25 09:02 totto82

I am fine with removing this if @steink agrees. But I have lately tested an approach that stops iterating (i.e. just give up) if it stagnates multiple times. This potentially gives some speedup but more testing is needed. Is is hard to add a communication step here to fix the parallel issue? You dont need to do it, I just want to know in case I pick this idea up at a later stage. And what about the similar code in iterateWellEqWithControl(...) does that create problems with MPI

You mean this part here? https://github.com/OPM/opm-simulators/blob/master/opm/simulators/wells/MultisegmentWell_impl.hpp#L1539, so far I have not seen problems with MPI at this part

lisajulia avatar Feb 20 '25 09:02 lisajulia

I just added a draft PR that start using this part of the code #6020 I think some code that detects stagnation / oscillation is a good idea.

Thanks!

lisajulia avatar Feb 20 '25 09:02 lisajulia

Closing this, since the issue was addressed in https://github.com/OPM/opm-simulators/pull/6020.

lisajulia avatar Apr 04 '25 05:04 lisajulia