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.

Sign up for GitHub

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

Jump to bottom

Use previous vfp table if default in historical runs #4228

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Use previous vfp table if default in historical runs #4228

merged 1 commit into from
Sep 25, 2024

Conversation

totto82
Copy link
Member

@totto82 totto82 commented Sep 25, 2024

For WCONHIST and WCONINJH default vfp table is the previous one

@totto82 totto82 force-pushed the fix_default_vfp_table branch from c1e50b6 to da61368 Compare September 25, 2024 09:32
@totto82 totto82 changed the title Use previous vfp table if default or 0 in historical runs Use previous vfp table if default in historical runs Sep 25, 2024
@totto82 totto82 marked this pull request as ready for review September 25, 2024 09:32
@totto82
Copy link
Member Author

totto82 commented Sep 25, 2024

jenkins build this please

@totto82 totto82 requested a review from bska September 25, 2024 09:33
@bska
Copy link
Member

bska commented Sep 25, 2024

jenkins build this failure_report please

bska
bska reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

I'm not opposed to this and I think we need to do something, but this does at least in part revert @vkip's commit bfa6199 (PR #4058) so I think we should discuss what the actual behaviour should be here.

@vkip
Copy link
Member

vkip commented Sep 25, 2024 •
edited
Loading

I'm not opposed to this and I think we need to do something, but this does at least in part revert @vkip's commit bfa6199 (PR #4058) so I think we should discuss what the actual behaviour should be here.

For prediction (WCONPROD) I believe it should be reset to 0 when defaulted, while using the previous value for WCONHIST is fine.

@bska
Copy link
Member

bska commented Sep 25, 2024

For prediction (WCONPROD) I believe it should be reset to 0 when defaulted, while using the previous value for WCONHIST is fine.

Okay, cool–thank you very much for your insight. In that case I'll update the reference solution for the failing regression test case and then I'll merge this.

@bska
Copy link
Member

bska commented Sep 25, 2024

jenkins build this update_data please

@jenkins4opm jenkins4opm mentioned this pull request Sep 25, 2024
Merged
jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request Sep 25, 2024
Automatic Reference Data Update for PR OPM/opm-common#4228
5f2a694 
Reason: PR OPM/opm-common#4228

opm-common     = 40c93f04ed16f6fb6c262f33a32fba092fda8f1e
opm-grid       = fc42f67a4b5f056d8fe3fb4560d63d6c911977fc
opm-models     = 40c93f04ed16f6fb6c262f33a32fba092fda8f1e
opm-simulators = 40c93f04ed16f6fb6c262f33a32fba092fda8f1e

### Changed Tests ###

  * actionx_wpimult
@bska
Copy link
Member

bska commented Sep 25, 2024

jenkins build this opm-tests=1232 please

bska added a commit to OPM/opm-tests that referenced this pull request Sep 25, 2024
@bska
Merge pull request #1232 from jenkins4opm/update_opm-common_4228
3767982 
Automatic Reference Data Update for PR OPM/opm-common#4228
bska
bska approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Good catch. The new reference solutions have been installed on the CI system so I'll merge this into the master branch now.

@bska bska merged commit 27aa07a into OPM:master Sep 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bska bska bska approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@totto82 @bska @vkip