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

Scale obs errors before outlier detection #5126

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Mar 22, 2023

Also scale before creating update snapshot so that scaled errors are printed to the deprecated file.

Note that the scaling of errors before outlier detection with global_std_scaling is wrong as it should be
sqrt(global_std_scaling) but this was how it was done before merging the feature branch that introduced the new storage solution.

I've tested this by running REEK locally with code before and after the merge of the storage feature branch, and see only insignificant differences.

A test should be written that tests the outlier detection but that will have to wait.
Here's an issue though: #5137

Issue
Resolves #5094

Approach
Short description of the approach

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Updated documentation
  • Ensured new behaviour is tested

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2023

Codecov Report

Merging #5126 (3682d5a) into main (85f76c2) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 3682d5a differs from pull request most recent head 3014a18. Consider uploading reports for the commit 3014a18 to get more accurate results

@@            Coverage Diff             @@
##             main    #5126      +/-   ##
==========================================
- Coverage   71.90%   71.87%   -0.04%     
==========================================
  Files         393      393              
  Lines       27403    27403              
  Branches     2027     2027              
==========================================
- Hits        19704    19695       -9     
- Misses       7131     7140       +9     
  Partials      568      568              
Impacted Files Coverage Δ
src/ert/analysis/_es_update.py 91.36% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dafeda dafeda self-assigned this Mar 22, 2023
@dafeda dafeda added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Mar 22, 2023
Also scale before creating update snapshot so that
scaled errors are printed to the deprecated file.

Note that the scaling of errors before outlier detection
with global_std_scaling is wrong as it should be
sqrt(global_std_scaling) but this was how it was done
before merging the feature branch that introduced the new
storage solution.
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

LGTM! Good detective work!

@dafeda dafeda merged commit 7cdf2d3 into equinor:main Mar 23, 2023
@dafeda dafeda deleted the reek_testing branch March 23, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:bug-fix Automatically categorise as bug fix in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Differences in HM-tutorial after merge of enkf_fs_refactor feature branch
3 participants