Fix general observations fmuobs for dictionaries

[daniel-sol]

General observations are not written when choosing yaml option, this PR Fixes this
Fixes issue #351

Updated info 18/1 2024.
Some background info. The general observations are different to the observation types in that they are not directly contained in the observation file, but contained in separate files only referenced in the observation file. This is seemingly relatively straight forward, with these files being simple text files containing two columns (without any header). The first being the observation value, and the second being the value error, or uncertainty, in one file generally called something.obs. Here a general observation line contains one reference to a file, and this is then treated as one atomic observation with a lable, corresponding roughly to summary observation.

But there is not really any proper datastructure behind this. This is clearly seen in the case of rft data which deviates strongly from this pattern, here to get the complete picture of what is going on there are are three files that need to be accessed per observation:

1. The observations, contained in a file with value and error pairs, e.g. observation.obs
2. The spatial assignment to the observations, in a twin file called observations.txt i.e x y md tvd zone (But no headers)
3. The name of the well contained in a file generally called well_dates.txt, but could be called something else entirely, see the some_field rft observations contained in the somewhere/completely/different/rft_ERT_use_MDadjusted/ folder.

There is really no logic, all this is patched together in an gen_data job in the ert config file.
To make it worse general observations can be used for absolutely anything, but there is nothing hindring people from bunging everything into the same folder as the observation file, but the convention, but not requirement from ert's side is that these datatypes are group into folders, with sometimes intelligible names. This straw to clutch is in this PR used as the key, because without it one would be completely in the dark for what the general observation is.

Hope this was somewhat informative, please get in touch if you want a chat..

[codecov-commenter]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (6e675c1) 86.13% compared to head (d43c9d0) 86.28%.
Report is 4 commits behind head on main.

❗ Current head d43c9d0 differs from pull request most recent head 6a4c3a0. Consider uploading reports for the commit 6a4c3a0 to get more accurate results

Files	Patch %	Lines
src/subscript/fmuobs/gen_obs_writers.py	92.12%	10 Missing ⚠️
src/subscript/pack_sim/pack_sim.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
+ Coverage   86.13%   86.28%   +0.15%     
==========================================
  Files          50       51       +1     
  Lines        7069     7234     +165     
==========================================
+ Hits         6089     6242     +153     
- Misses        980      992      +12     

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

[mferrera]

This PR adds a significant amount of text test data. It is a bit hard to tell because there are quite a few files, but are all of them needed?

[daniel-sol]

This PR adds a significant amount of text test data. It is a bit hard to tell because there are quite a few files, but are all of them needed?
Most of the files are needed, but I will remove some of them, e.g the files in folder welltest. They are not used in the testing. Regarding the other files I would rather keep them for some granularity, but just get in touch if it is unclear what they do..

[alifbe]

Hi @daniel-sol

Instead of assuming observation type based on folder name, we would recommend to probably let user specify it directly. It could be in the form of dictionary file that map observation key to observation type. Or perhaps a comment in ERT observation config. This will ensure that there is no guessing which might be prone to error.

The code can be extended to give warning when observation is not defined in the dictionary or vice versa when it exist in the dictionary but not in the ERT observation file.

[daniel-sol]

Hi @daniel-sol

Instead of assuming observation type based on folder name, we would recommend to probably let user specify it directly. It could be in the form of dictionary file that map observation key to observation type. Or perhaps a comment in ERT observation config. This will ensure that there is no guessing which might be prone to error.

The code can be extended to give warning when observation is not defined in the dictionary or vice versa when it exist in the dictionary but not in the ERT observation file.

I am a bit reluctant to do that. I do see your point, but one of my philosophies is to not add too much to the burden of the end user. This would be one of those things, what we could to is to introduce something an option to add something to your fmu-config if you want to fix it? But I don't want it to be a requirement.

See last commit. Where I have added the possibility of changing names from the fmu_config, and I have also added that if a user hasn't put the file in a folder it will end up with a key called unspecified, and with a strong warning..

[asnyv]

Agree with @alifbe that the test data here seems way larger than needed. Most of it seems like with repeated lines of the same formatting, and/or several files of the same type? Has to be possible to cut these drastically?
In most cases it should be possible to test the same thing with 5 lines of test data as with 15000 lines.
The length of the data also makes it cumbersome to look at whether the setup seems reasonable.
Think we should also test this locally with as many different models as possible prior to merge to see if the current solution is robust.

[alifbe]

@daniel-sol could you please rebase your PR to main branch? I think the workflow is not triggered anymore since your PR went stale.

[anders-kiaer]

Will have a chat with @sondreso on the GENOBS topic in February. Suggestion from Daniel and myself is that we wait with merge + adding GENOBS support until after that meeting 🙂

[anders-kiaer]

FYI: Summary of yesterday's workshop (which included representatives from ERT, fmu-dataio, SUMO, webviz and subscript) on observations is summarized in #683 by @daniel-sol and myself.

[anders-kiaer]

This PR will be superseded by a new PR currently in the making.