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

Remove pre balance of network #6014

Closed
wants to merge 1 commit into from
Closed

Conversation

totto82
Copy link
Member

@totto82 totto82 commented Feb 19, 2025

I dont think the prebalance step is needed anymore after subiterations are added. This need to be tested more. Testing needs to be done together with #6011

@totto82 totto82 marked this pull request as draft February 19, 2025 10:12
@totto82 totto82 requested a review from GitPaean February 19, 2025 10:12
@totto82
Copy link
Member Author

totto82 commented Feb 19, 2025

jenkins build this please

@GitPaean
Copy link
Member

jenkins build this failure_report please

1 similar comment
@GitPaean
Copy link
Member

jenkins build this failure_report please

@atgeirr
Copy link
Member

atgeirr commented Feb 19, 2025

What about doing a simplified solve (using just implicit IPRs) for the wells in this pre-balance step to have a cheap way to get close to the true solution before the proper balancing loop starts? Also in that case: we should probably run this code more often than we do, currently we require a status change event for some well whereas it could be done for all timesteps to save iterations.

Finally, if we instead continue with deleting this method, we should also remove BlackoilWellModelGeneric::needPreStepNetworkRebalance().

@totto82
Copy link
Member Author

totto82 commented Feb 20, 2025

What about doing a simplified solve (using just implicit IPRs) for the wells in this pre-balance step to have a cheap way to get close to the true solution before the proper balancing loop starts?

I did a lot of testing with simplified solves like IPR / STW etc. I still have the code so maybe we can revisit this idea later. But it didn't give much effect. The simplest and best solution seems to be to just remove it. I made it a draft since I would like to know the motivation for this code, but as it is now it adds a huge cost for some models. If you have an example of a model where this has a positive effect we could add it as an option via a parameter. This also make it easier to consider improvements later.

@GitPaean
Copy link
Member

I made it a draft since I would like to know the motivation for this code, but as it is now it adds a huge cost for some models.

I am out of office but I can answer this. The motivation is that some situation changed with the wells in the network, we will do a re-balance of the network. It is similar to the solveWellEq that we pre-solve the well equations so the later iteration will be easier since the well/network is with a better initial guess.

@totto82
Copy link
Member Author

totto82 commented Mar 3, 2025

I am out of office but I can answer this. The motivation is that some situation changed with the wells in the network, we will do a re-balance of the network. It is similar to the solveWellEq that we pre-solve the well equations so the later iteration will be easier since the well/network is with a better initial guess

Since it is still a possibility that the pre-balance has a positive impact on some models I will close this PR and instead provide a PR where the pre balancing is optional. --enable-pre-balance-network (default to true) to make all tests happy.

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