Move EclSum support from serializers to transformations #2613

berland · 2021-12-17T12:39:14Z

Issue
Resolves #2121

Approach
Short description of the approach

Pre review checklist

Added appropriate labels

ert3_examples/spe1/stages.yml

ert/data/record/_record.py

codecov-commenter · 2021-12-17T13:30:49Z

Codecov Report

Merging #2613 (94d7347) into main (17af879) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 94d7347 differs from pull request most recent head 73d6cf2. Consider uploading reports for the commit 73d6cf2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2613      +/-   ##
==========================================
- Coverage   64.82%   64.82%   -0.01%     
==========================================
  Files         648      648              
  Lines       54188    54188              
  Branches     4517     4517              
==========================================
- Hits        35128    35127       -1     
- Misses      17653    17655       +2     
+ Partials     1407     1406       -1

Impacted Files	Coverage Δ
ert3/algorithms/_sensitivity.py	`90.29% <100.00%> (ø)`
ert3/config/_ensemble_config.py	`96.82% <100.00%> (ø)`
ert_gui/simulation/run_dialog.py	`75.70% <0.00%> (-0.71%)`	⬇️
libres/lib/enkf/block_fs_driver.cpp	`81.92% <0.00%> (+0.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17af879...73d6cf2. Read the comment docs.

ert3/config/_ensemble_config.py

ert/data/record/_record.py

ert3/evaluator/_builder.py

ert/data/record/_transformation.py

xjules · 2022-01-04T09:15:07Z

Another mypy issues:

ert3/config/_ensemble_config.py:48: error: unused "type: ignore" comment
ert3/algorithms/_sensitivity.py:109: error: unused "type: ignore" comment

xjules · 2022-01-04T09:43:24Z

ert3/evaluator/_builder.py

@@ -106,16 +102,13 @@ def add_step_outputs(
    step: StepBuilder,
 ) -> None:
    for record_name, output in step_config.output.items():
+        transformation = _get_output_recordtransformation(output)


This might not be part of this PR, but shouldn't we also create LinkedOutput class then?

The outputs don't have the same kind of configurability as the inputs, so it isn't currently needed. The reason linked inputs exist, is due to the separate stages and ensemble configuration scheme, where the same input (record) can serve as inputs for multiple ensembles. This doesn't currently apply for outputs.

xjules · 2022-01-04T10:15:10Z

Another mypy issues:

ert3/config/_ensemble_config.py:48: error: unused "type: ignore" comment
ert3/algorithms/_sensitivity.py:109: error: unused "type: ignore" comment

It's been fixed in 94d7347 it seems, so just rebase

tests/ert_tests/ert3/data/test_transformations.py

sondreso

Looks very good! Only had one small comment.

ert/data/record/_transformation.py

Eclipse binary output can now be "transformed" from its binary state into a NumericalRecordTree with nodes being the summary vector names. Summary vectors are extracted according to an explicit list.

jondequinor · 2022-01-05T07:51:28Z

Can I still load summary data from the command line after this pr?

xjules · 2022-01-05T07:54:43Z

Can I still load summary data from the command line after this pr?

Very good point (although not sure if we were loading the summary data from command line before 🤔 )!
Regardless, I think we should extend the command line interface for summary data too (at least in another PR / creating an issue for it).
See is-directory for example:

ert/ert3/console/_console.py

Line 73 in c1f45fb

"--is-directory",

jondequinor · 2022-01-05T08:05:31Z

Very good point (although not sure if we were loading the summary data from command line before 🤔 )!

The command line dispatched stuff to the serialiser, so that should've worked before this pr. didn't test it though.

xjules · 2022-01-05T13:23:04Z

Very good point (although not sure if we were loading the summary data from command line before thinking )!

The command line dispatched stuff to the serialiser, so that should've worked before this pr. didn't test it though.

According to the previous experience, this feature didn't work (ie. extracting particular keys). Moreover, decided not to have a command line option for summary data (at least for now).

xjules

Nice work and LGTM now! 🚀

berland marked this pull request as draft December 17, 2021 12:39

sondreso reviewed Dec 17, 2021

View reviewed changes

ert3_examples/spe1/stages.yml Outdated Show resolved Hide resolved

jondequinor reviewed Dec 17, 2021

View reviewed changes

ert/data/record/_record.py Show resolved Hide resolved

jondequinor reviewed Dec 17, 2021

View reviewed changes

ert3/config/_ensemble_config.py Show resolved Hide resolved

sondreso changed the title ~~Smry trans~~ Summary transformation Dec 17, 2021

berland added the ert3 label Dec 21, 2021

This was linked to issues Dec 21, 2021

EclSum instantiation is blocking in an async function #2601

Closed

Make Summary vector transformation #2121

Closed

Use summary transformation in SPE1 example #2514

Closed

berland force-pushed the smry_trans branch 2 times, most recently from b8ec2dd to a040e4c Compare December 21, 2021 13:27

xjules reviewed Dec 21, 2021

View reviewed changes

ert/data/record/_record.py Show resolved Hide resolved

xjules reviewed Dec 22, 2021

View reviewed changes

ert/data/record/_record.py Show resolved Hide resolved

berland changed the title ~~Summary transformation~~ Move EclSum support from serializers to transformations Dec 22, 2021

xjules mentioned this pull request Dec 23, 2021

Initial draft for SMRY transformations #2551

Closed

xjules reviewed Dec 23, 2021

View reviewed changes

ert3/evaluator/_builder.py Outdated Show resolved Hide resolved

jondequinor reviewed Dec 23, 2021

View reviewed changes

ert/data/record/_transformation.py Outdated Show resolved Hide resolved

jondequinor reviewed Dec 23, 2021

View reviewed changes

ert/data/record/_transformation.py Outdated Show resolved Hide resolved

berland force-pushed the smry_trans branch from 25b7a32 to a10d33c Compare December 27, 2021 17:08

jondequinor reviewed Dec 27, 2021

View reviewed changes

ert/data/record/_transformation.py Outdated Show resolved Hide resolved

ert/data/record/_transformation.py Show resolved Hide resolved

berland force-pushed the smry_trans branch from 4e6a014 to f539c0f Compare January 3, 2022 08:43

berland marked this pull request as ready for review January 4, 2022 08:23

xjules reviewed Jan 4, 2022

View reviewed changes

tests/ert_tests/ert3/data/test_transformations.py Show resolved Hide resolved

sondreso reviewed Jan 4, 2022

View reviewed changes

ert/data/record/_transformation.py Outdated Show resolved Hide resolved

Move EclSum support from serializers to transformations.

ac3b064

Eclipse binary output can now be "transformed" from its binary state into a NumericalRecordTree with nodes being the summary vector names. Summary vectors are extracted according to an explicit list.

berland force-pushed the smry_trans branch from 73d6cf2 to ac3b064 Compare January 5, 2022 07:00

xjules approved these changes Jan 5, 2022

View reviewed changes

berland merged commit 19068cc into equinor:main Jan 5, 2022

berland deleted the smry_trans branch January 7, 2022 14:58

berland mentioned this pull request Jan 11, 2022

Fix SPE1 yaml config for smry_keys #2685

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move EclSum support from serializers to transformations #2613

Move EclSum support from serializers to transformations #2613

berland commented Dec 17, 2021 •

edited

codecov-commenter commented Dec 17, 2021 •

edited

xjules commented Jan 4, 2022

xjules Jan 4, 2022

jondequinor Jan 4, 2022 •

edited

xjules commented Jan 4, 2022

sondreso left a comment

jondequinor commented Jan 5, 2022

xjules commented Jan 5, 2022 •

edited

jondequinor commented Jan 5, 2022

xjules commented Jan 5, 2022

xjules left a comment

Move EclSum support from serializers to transformations #2613

Move EclSum support from serializers to transformations #2613

Conversation

berland commented Dec 17, 2021 • edited

Pre review checklist

codecov-commenter commented Dec 17, 2021 • edited

Codecov Report

xjules commented Jan 4, 2022

xjules Jan 4, 2022

Choose a reason for hiding this comment

jondequinor Jan 4, 2022 • edited

Choose a reason for hiding this comment

xjules commented Jan 4, 2022

sondreso left a comment

Choose a reason for hiding this comment

jondequinor commented Jan 5, 2022

xjules commented Jan 5, 2022 • edited

jondequinor commented Jan 5, 2022

xjules commented Jan 5, 2022

xjules left a comment

Choose a reason for hiding this comment

berland commented Dec 17, 2021 •

edited

codecov-commenter commented Dec 17, 2021 •

edited

jondequinor Jan 4, 2022 •

edited

xjules commented Jan 5, 2022 •

edited