Skip to content

Do not flag identical negative numbers as different. #4490

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

Merged

Conversation

atgeirr
Copy link
Member

@atgeirr atgeirr commented Feb 19, 2025

This applies to the comparison program that checks regression failures. With the current approach, keywords in the "keywordDisallowNegatives" set will be treated such that even if the reference and test results are identical, if negative it will still be flagged as a failing test (if the negative values are larger than the absolute tolerance). This is unproductive when applied to the regression tests, and just gives unnecessary failures. If the intention is to disallow any negative values at all in certain fields, the code must be changed to do just that (and not allow negative values with small absolute value), I would support a move in that direction, although it would also require modification to Flow, which currently can indeed produce negative values for SGAS and SWAT.

The change proposed here is a simple disabling of this (in my opinion) strange behavior, but we could discuss more fundamental changes as outlined above.

@atgeirr
Copy link
Member Author

atgeirr commented Feb 27, 2025

Any opinions on this, @akva2 @bska @totto82?

@bska
Copy link
Member

bska commented Feb 27, 2025

Any opinions on this,

Yeah, I think it makes sense to disable that behaviour. Tracing the history, this mode seems to have been present since commit 51c7a97 (originally OPM/opm-output@a2897a2, arguably since OPM/opm-output@4aa250e, OPM/opm-output#55) and never really justified anywhere.

I believe the original goal was to prevent including reference solutions with negative saturations &c, but since we've since added such solutions it has not met its goal. Altering the comparison to accept negative values as long as they don't change (too much) is probably beneficial in the short term. Long term I do think we should strive towards never generating such negative values, but that's a much bigger program.

@bska
Copy link
Member

bska commented Feb 27, 2025

jenkins build this please

1 similar comment
@atgeirr
Copy link
Member Author

atgeirr commented Apr 23, 2025

jenkins build this please

@atgeirr
Copy link
Member Author

atgeirr commented Apr 24, 2025

@bska ok to merge this?

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.

ok to merge this?

Yes I think so. It simplifies our task in the short term, but the long-term solution is likely to be more involved. I'll merge into master now.

@bska bska merged commit 7a151fe into OPM:master Apr 25, 2025
1 check passed
@atgeirr atgeirr deleted the allow-negative-vals-for-regression-comparison branch May 12, 2025 11:45
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.

2 participants