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

Clean up: remove code in if(false) branch #6016

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

Conversation

lisajulia
Copy link
Contributor

This logic was added in commit 035d216 (PR #4895).
@steink: Can you have a look at this?

@lisajulia
Copy link
Contributor Author

jenkins build this please

@totto82
Copy link
Member

totto82 commented Feb 19, 2025

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

@lisajulia
Copy link
Contributor Author

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 lisajulia closed this Feb 19, 2025
@steink
Copy link
Contributor

steink commented Feb 19, 2025

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.

@lisajulia lisajulia reopened this Feb 20, 2025
@lisajulia
Copy link
Contributor Author

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?

@totto82
Copy link
Member

totto82 commented Feb 20, 2025

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.

@lisajulia
Copy link
Contributor Author

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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants