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

Use well rates from well_state_nupcol instead of previous_well_state in updateWellRates #6128

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

totto82
Copy link
Member

@totto82 totto82 commented Apr 1, 2025

The previous_well_state was used to avoid oscillations long time ago. Using the nupcol rates aligns more with what we do elsewhere in the code and will be more accurate in particular at startup of wells. Test failures are expected. A qualitative look at the failed tests indicates that results looks more plausible.

@totto82
Copy link
Member Author

totto82 commented Apr 1, 2025

jenkins build this failure_report please

@totto82 totto82 changed the title FOR TESTING: Use well rates from well_state_nupcol instead of previous_well_state in updateWellRates Use well rates from well_state_nupcol instead of previous_well_state in updateWellRates Apr 1, 2025
@totto82 totto82 marked this pull request as ready for review April 1, 2025 09:30
@vkip
Copy link
Member

vkip commented Apr 1, 2025

For what it's worth: I had a quick look at one of the more severe test failures (0A4_GRCTRL_LRAT_LRAT_GGR_BASE_MODEL2_MSW.DATA), and the behavior with this PR is much more reasonable than with master. Specifically, the early time results are closer to what is obtained with a very short initial time-step.

@totto82
Copy link
Member Author

totto82 commented Apr 1, 2025

the behavior with this PR is much more reasonable than with master

I agree. I implemented this some years ago. Back then, it was necessary to avoid severe oscillations, but I don't see that issue anymore. In any case, setting NUPCOL to something reasonable, like 3-5, is a better workaround IMO. Should we try to get this in before the release?

@totto82 totto82 requested a review from GitPaean April 1, 2025 14:11
@totto82 totto82 added this to the Release 2025.04 milestone Apr 1, 2025
@vkip
Copy link
Member

vkip commented Apr 1, 2025

Should we try to get this in before the release?

IMHO, yes.

@GitPaean
Copy link
Member

GitPaean commented Apr 1, 2025

Thanks for the request. It is something interesting to see that the function prototype is using wellStateNupcol, while when we use it, it uses the prevWellState(). And all the functions around are using well_state_nupcol.

I will invite @steink regarding this, he might have more insight regarding this part.

@GitPaean GitPaean requested a review from steink April 1, 2025 15:06
Copy link
Member

@GitPaean GitPaean left a comment

Choose a reason for hiding this comment

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

With more learning and thinking, this is probably fine and should follow what with testing indicates. Not working on this area much and not familiar with the exact comparison between the two different well states. It is difficult for me to understand deeply how it affects the output exactly. Will wait for @steink 's comments, otherwise, it should be able to go in.

@steink
Copy link
Contributor

steink commented Apr 2, 2025

This PR seems very reasonnable to me. Will just do a test-run, then report back.

Copy link
Contributor

@steink steink left a comment

Choose a reason for hiding this comment

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

@GitPaean I'm good with this and think it should go in

@GitPaean
Copy link
Member

GitPaean commented Apr 2, 2025

jenkins build this update_data please

jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request Apr 2, 2025
Reason: PR OPM/opm-simulators#6128

opm-common     = 771e89b8b9ee76ca3789559e687c6582861e9ecf
opm-grid       = a8afffe952be60f86a5ae60f562a52e68aec22dd
opm-simulators = cf97d4cab3693139e7343c192b634f1a102d4cae

### Changed Tests ###

  * 0a1_grpctl_stw_model2
  * 0a1_grpctl_msw_model2
  * 0a2_grpctl_stw_model2
  * 0a2_grpctl_msw_model2
  * 0a3_grpctl_stw_model2
  * 0a3_grpctl_msw_model2
  * 0a4_grpctl_stw_model2
  * 0a4_grpctl_msw_model2
  * udq_actionx
  * udq_wconprod
  * udq_pyaction
  * model4_udq_group
  * model4_gefac
@GitPaean
Copy link
Member

GitPaean commented Apr 2, 2025

jenkins build this opm-tests=1321 please

GitPaean added a commit to OPM/opm-tests that referenced this pull request Apr 2, 2025
@GitPaean
Copy link
Member

GitPaean commented Apr 2, 2025

the reference has been updated. I am getting the PR in now.

@GitPaean GitPaean merged commit 5c17f27 into OPM:master Apr 2, 2025
1 check passed
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.

4 participants