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

Set option maximum-number-of-well-switches default value to 10 #5599

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

Conversation

steink
Copy link
Contributor

@steink steink commented Sep 11, 2024

Increasing default value of option --maximum-number-of-well-switches from 3 to 10. Fixes a few problematic cases and otherwise do not seem to do any harm (performance-wise). Will break many tests, though (will go through them).

@steink
Copy link
Contributor Author

steink commented Sep 11, 2024

jenkins build this please

@akva2
Copy link
Member

akva2 commented Sep 11, 2024

(just in case: you need to include failure_report in the trigger for the pdfs to be generated)

@steink
Copy link
Contributor Author

steink commented Sep 11, 2024

(just in case: you need to include failure_report in the trigger for the pdfs to be generated)

Argh, yes forgot

@steink
Copy link
Contributor Author

steink commented Sep 11, 2024

jenkins build this failure_report please

@akva2
Copy link
Member

akva2 commented Sep 11, 2024

build will fail because #5598 was not yet merged.

@steink
Copy link
Contributor Author

steink commented Sep 11, 2024

build will fail because #5598 was not yet merged.

OK, so build with opm-simulators=5598?

@akva2
Copy link
Member

akva2 commented Sep 11, 2024

no can't have two pr's for opm-simulators... have to wait for it to be merged.

@akva2
Copy link
Member

akva2 commented Sep 11, 2024

which is now done.

@akva2
Copy link
Member

akva2 commented Sep 11, 2024

jenkins build this failure_report please

@GitPaean
Copy link
Member

It looks like when we click on view, we are opening the raw pdf file? which is not very helpful.

image

https://ci.opm-project.org/job/opm-simulators-PR-builder/6624/artifact/mpi/build-opm-simulators/failure_report/01_udq_pyaction.pdf/*view*/

I would suggest when we click on view, we say some output from the compareECL.

@akva2
Copy link
Member

akva2 commented Sep 12, 2024

don't click on view, click on the filename. view is only useful for ascii files. i cannot control that. you can see the compareECL output by looking at the output of the failed test. this is just how jenkins works, all i do is attach pdf artifacts.

@GitPaean
Copy link
Member

don't click on view, click on the filename....

I figured that out, but since the view button is there...

@GitPaean
Copy link
Member

don't click on view, click on the filename. view is only useful for ascii files. i cannot control that. you can see the compareECL output by looking at the output of the failed test. this is just how jenkins works, all i do is attach pdf artifacts.

Thanks for the explanation. I have no ideas about how the things on Jenkins work.

I see I can still see the compareECL output in the original place. A potential small improvements is that the pdf and the compareECL output can be in the same line. But I have no ideas what efforts it might cause.

But no worries, in its current form it is already a huge help for investigating jenkins regression, which is much appreciated.

@akva2
Copy link
Member

akva2 commented Sep 12, 2024

it would mean digging into the java code of jenkins and/or writing a new groovy or java based plugin for jenkins. it would be a rather involved effort.

@totto82
Copy link
Member

totto82 commented Sep 12, 2024

The maximum-number-of-well .. options was added to avoid shutting of wells some time ago. I think it is still needed for some cases to avoid issues. The downside is that the simulator now can accept solution where lot of the well/group constrains are broken. For instance thp << thp_limit. Some time ago I did some work to fix this more properly. See #3410. Maybe it is time to resurrect that work. @steink Could you elaborate a bit on which models this fixes? Either here or by email.

@steink
Copy link
Contributor Author

steink commented Sep 12, 2024

The maximum-number-of-well .. options was added to avoid shutting of wells some time ago. I think it is still needed for some cases to avoid issues. The downside is that the simulator now can accept solution where lot of the well/group constrains are broken. For instance thp << thp_limit. Some time ago I did some work to fix this more properly. See #3410. Maybe it is time to resurrect that work. @steink Could you elaborate a bit on which models this fixes? Either here or by email.

With the new defaults, we seem to be a bit more sensitive to this option. One example is the standard Norne test-model where one well causes a lot of problems because it is never able to switch to the correct control due to this limit (due to oscillations in surrounding reservoir pressure during Newton iterations). I'll send you some more background. I'll have a look at #3410 as this seems to be very relevant for a different but related problem I'm looking into.

@totto82
Copy link
Member

totto82 commented Sep 12, 2024

I can see if I am able to rebase #3410 on current master and make it ready again. The number 3 was a bit ad-hoc, so I am not against changing the default number. 10 will in essence turn it off since we only allow one switch per newton iteration and the maximum newton is typically <= 20.

@alfbr
Copy link
Member

alfbr commented Sep 12, 2024

The number 10 was a bit arbitrary from our side too. From our testing we could just as well go with 5.

@totto82
Copy link
Member

totto82 commented Sep 13, 2024

Maybe a more robust logic here is to set it to the individual constrain when oscillating. i.e. for a well that oscillates between well and group control, we force to use the well control if oscillating more than X number of times.

@totto82 totto82 mentioned this pull request Sep 13, 2024
@steink
Copy link
Contributor Author

steink commented Sep 13, 2024

Maybe a more robust logic here is to set it to the individual constrain when oscillating. i.e. for a well that oscillates between well and group control, we force to use the well control if oscillating more than X number of times.

Good point. In addition, I think when oscillating between pressure (bhp or thp) and some rate one should force to use pressure control since a rate control might not be feasible which in turn may lead to convergence issues or problematic pressures. So if a well is oscillating between two controls, we force the control with highest priority according to e.g., 1. bhp, 2. thp, 3, various well rates, 4. group control?

@totto82
Copy link
Member

totto82 commented Sep 13, 2024

Good point. In addition, I think when oscillating between pressure (bhp or thp) and some rate one should force to use pressure control since a rate control might not be feasible which in turn may lead to convergence issues or problematic pressures. So if a well is oscillating between two controls, we force the control with highest priority according to e.g., 1. bhp, 2. thp, 3, various well rates, 4. group control?

I can test it out in the #5607 branch. For the GASLIFT-12 case I am working on the controls switch between ORAT and THP and is kept at ORAT which gives strange results. I can report back when I have tested it.

@steink steink marked this pull request as draft September 13, 2024 13:53
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.

5 participants