Skip to content

Fix rounding logic in test_field_parameter#12715

Merged
berland merged 1 commit intoequinor:mainfrom
berland:fix_field_parameter_snapshot_test
Jan 23, 2026
Merged

Fix rounding logic in test_field_parameter#12715
berland merged 1 commit intoequinor:mainfrom
berland:fix_field_parameter_snapshot_test

Conversation

@berland
Copy link
Contributor

@berland berland commented Jan 23, 2026

The previous code was called with outlier_threshold having a typical value of 0.01, which makes polars round to 100 decimal places. This causes almost certain failures when trying to update the snapshot

After the change, an outlier_threshold at 0.01 means that we round to 2 decimal place.

The snapshot is updated. This snapshot was never valid, as the bug fixed in 9d1325f hid it.

Issue
Resolves #12713

Approach
🧠

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

The previous code was called with outlier_threshold having a typical
value of 0.01, which makes polars round to 100 decimal places. This
causes almost certain failures when trying to update the snapshot

After the change, an outlier_threshold at 0.01 means that we round to 2
decimal place.

The snapshot is updated. This snapshot was never valid, as the bug fixed
in 9d1325f hid it.
@berland
Copy link
Contributor Author

berland commented Jan 23, 2026

@yngve-sk, the change is educated guesswork on how the parameter outlier_threshold was supposed to be used.

@berland berland added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Jan 23, 2026
@berland berland self-assigned this Jan 23, 2026
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.63%. Comparing base (03914d2) to head (1ed19a0).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12715      +/-   ##
==========================================
- Coverage   90.64%   90.63%   -0.01%     
==========================================
  Files         431      431              
  Lines       30113    30113              
==========================================
- Hits        27296    27293       -3     
- Misses       2817     2820       +3     
Flag Coverage Δ
cli-tests 37.30% <ø> (ø)
gui-tests 68.67% <ø> (ø)
performance-and-unit-tests 74.06% <ø> (-0.01%) ⬇️
test 37.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 23, 2026

Merging this PR will not alter performance

✅ 23 untouched benchmarks


Comparing berland:fix_field_parameter_snapshot_test (1ed19a0) with main (03914d2)

Open in CodSpeed

def round_and_cast_(df: pl.DataFrame) -> pl.DataFrame:
return df.cast(_schema).with_columns(
pl.col(pl.Float64).round(int(1 / outlier_threshold))
pl.col(pl.Float64).round(int(-math.log10(outlier_threshold)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: outlier threshold is always <1 so log10 always negative, but perhaps still do abs instead of -?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using - makes it valid without the assumption that outlier_threshold < 1 so I think keeping - makes the code clearer.

Copy link
Contributor

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

LGTM, only small nitpick suggestion

@yngve-sk
Copy link
Contributor

yngve-sk commented Jan 23, 2026

the change is educated guesswork on how the parameter outlier_threshold was supposed to be used

(Yes the method is quite ad hoc and authored by myself). The outlier_threshold is the max absolute difference between expected and actual number when comparing the snapshots, but was also intended to be used to round the snapshots before saving (as it sort of is the same info, about the precision of the comparison, which relates to the precision that should be used to store the reference snapshot), which is where it (as you found) did not make sense.

Behavior on main:

  • Threshold of 0.5 will define 0.5 as max deviation and store snapshots rounded to 2 decimals
  • Threshold of 0.05 will define .05 ax max deviation and store snapshots rounded to 20 decimals
    ... does not make sense

Behavior in this PR (by simple example):

  • For example threshold of 0.1 will round to 1 decimal for the comparison, and use 0.1 as a threshold to define max diff between expected and actual.
  • Threshold of 0.5 will define 0.5 as max deviation .round to int(0.3) 0 decimals, which is really 0 decimals for the comparison.
  • Threshold of 0.05 will define .05 ax max deviation, and round to 1.3 ~ 1 decimals... So it sort of works but not perfect.

So summary from a quick call/discussion with @berland : There is probably a better way to do this, ref ERT internal examples comparison. The intent of this is to do an approx comparison of some numbers and update snapshots based on that. It is not at the moment worth pursuing a fix on this beyond what is done in this PR.

@berland berland merged commit fe45493 into equinor:main Jan 23, 2026
36 checks passed
@berland berland deleted the fix_field_parameter_snapshot_test branch March 6, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes:maintenance Automatically categorise as maintenance change in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests/ert/ui_tests/cli/test_field_parameter.py::test_field_param_update_using_heat_equation_enif_snapshot not working

3 participants