Migrate surfaces over to binary format#12840
Migrate surfaces over to binary format#12840jonathan-eq wants to merge 4 commits intoequinor:mainfrom
Conversation
0c2cb0c to
475a8c6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12840 +/- ##
==========================================
- Coverage 90.12% 90.11% -0.02%
==========================================
Files 447 447
Lines 30946 30967 +21
==========================================
+ Hits 27890 27905 +15
- Misses 3056 3062 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
a074e70 to
14c8ab6
Compare
src/ert/config/surface_config.py
Outdated
| file_format = SurfaceFileFormat.BINARY | ||
| try: | ||
| surf = IrapSurface.from_ascii_file(Path(base_surface)) | ||
| try: | ||
| surf = IrapSurface.from_binary_file(Path(base_surface)) | ||
| logger.info("Loaded surface in binary format") | ||
| except Exception: | ||
| surf = IrapSurface.from_ascii_file(Path(base_surface)) | ||
| ConfigWarning.warn(ASCII_SURFACE_WARNING_MESSAGE) | ||
| file_format = SurfaceFileFormat.ASCII |
There was a problem hiding this comment.
The idea here is that if you have binary on the initial validation, then you stick with the binary version when writing/reading from runpath. If you still use ascii, you get the warning, but ert will continue as before, thus not breaking any forward models that still read/write ascii surfaces
14c8ab6 to
e7d45a3
Compare
b415594 to
4ff8eaf
Compare
| super().__init__(msg) | ||
|
|
||
|
|
||
| ASCII_SURFACE_WARNING_MESSAGE = ( |
There was a problem hiding this comment.
We should probably add something about this in the docs aswell
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("is_ascii_surface", [True, False]) |
There was a problem hiding this comment.
Could we add a test that tests if we read ascii we also write ascii
src/ert/config/surface_config.py
Outdated
|
|
||
|
|
||
| ASCII_SURFACE_WARNING_MESSAGE = ( | ||
| "From this release, the default format for surfaces is changing from ASCII " |
There was a problem hiding this comment.
I'd remove "From this release" :)
And refactor the text a bit
|
What about storage migration? ... |
src/ert/config/surface_config.py
Outdated
| ASCII_SURFACE_WARNING_MESSAGE = ( | ||
| "The default format for surfaces is changing from ASCII " | ||
| "to binary, but Ert will temporarily still support ASCII surfaces. Binary " | ||
| "surfaces are recommended, as it is faster to write and read from runpath." |
There was a problem hiding this comment.
Let's add a sentence that the forward model creating the surfaces need to be changed too to export surfaces in binary format.
|
Added a note section in the docs, is this enough? @frode-aarstad |
2a32793 to
cd9d8a1
Compare
cd9d8a1 to
d9b3d6b
Compare
src/ert/config/surface_config.py
Outdated
| base_surface_path=base_surface, | ||
| update=update_parameter, | ||
| ) | ||
| instance._file_format = file_format |
There was a problem hiding this comment.
Dont think this will work, because when we read from storage this will never be set, as an example, if you change:
toirap_ascii I would expect this setting to be: ascii, but it is binary when we go to read the file from runpath.
There was a problem hiding this comment.
No sure I understand. Base surface creates surf.irap that is binary and we still gonna read it from runpath as binary (by default). So binary all the way.
There was a problem hiding this comment.
I think this needs more discussion @xjules @oyvindeide
If I set the default to ASCII, then old storages will be handled. If there is no key for file_format in parameters.json, then it will use ASCII and write file_format: ASCII to parameters.json and that will be picked up on the next reading. If there is a new field already in binary format, that will write file_format: BINARY to parameters.json, and that will also work downstream.
src/ert/config/surface_config.py
Outdated
| @@ -59,6 +78,8 @@ class SurfaceConfig(ParameterConfig): | |||
| output_file: Path | |||
| base_surface_path: str | |||
|
|
|||
There was a problem hiding this comment.
These file names should be mentioned explicitly in the warning message.
20437c4 to
da961a5
Compare
Before this commit, surfaces are only read by ert in ascii format. This commit changes the default to be binary, but falls back to ascii if it fails. It will also emit warnings if binary reading fails.
da961a5 to
1a079ec
Compare
ed4aff4 to
49e32d4
Compare


Issue
Resolves #12824
Approach
Before this commit, surfaces are read by ert in ascii format. This commit changes the default to be binary, with the option to override by setting
SURFACE .... FORMAT:ASCIIin config. This will retain the SURFACE old behavior.(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable