ENH: add data.geometry for option to link to geometry #635

jcrivenaes · 2024-04-25T10:52:57Z

Replaces older un-merged PR #528 (but see discussion there).

This is particularly useful for a GridProperty, so we can refer to the grid object. But it can also be used for e.g. surfaces when wanting to link e.g. a property to a depth surface, for "draping" in WebViz.

Example stubs

gridfile = edata.export(grid_object)

porofile = edata.export(poro_object, geometry=gridfile)

It will then create in the metadata:

data:
   geometry:
      name: "mygrid"
      relative_path: "some/path/to/grid.roff"

This is particulary useful for a GridProperty, so we can refer to the grid object. But it can also be used for e.g. surfaces when wanting to link e.g. a property to a depth surface, for "draping" in WebViz.

perolavsvendsen · 2024-04-25T11:03:52Z

Readability of the datamodel: For someone without prior knowledge, data.geometry could potentially be understood as "the geometry of these data". Aka similar to data.spec. Perhaps it should be data.relations.geometry or similar? (or possibly just relations.geometry on root level, next to data?)

perolavsvendsen · 2024-04-25T11:04:56Z

src/fmu/dataio/_utils.py

+        relative_path: some_relative/path/geometry.roff
+
+    This means that the geometry may be 'located' both on disk (relative path) and in
+    SUMO


*Sumo, not SUMO :)

Ref section 8, paragraph 24 of the Sumo brand guidelines (revision 6): "Sumo is a name, not an abbreviation."

perolavsvendsen · 2024-04-25T11:08:18Z

tests/test_units/test_rms_context.py

+    os.chdir(rmssetup)
+
+    edata = dataio.ExportData(
+        config=rmsglobalconfig, content="depth"


Should the content be more granular then "depth"? Perhaps add grid_geometry as a content, not sure. But depth feels wrong. (data.vertical_domain defines if this is depth or time.)

perolavsvendsen · 2024-04-25T11:10:06Z

src/fmu/dataio/providers/objectdata/_xtgeo.py

@@ -354,5 +359,6 @@ def get_objectdata(self) -> DerivedObjectDescriptor:
            ),
            spec=self.get_spec(),
            bbox=self.get_bbox() or None,
+            geometry=get_geometry_ref(self.dataio.geometry, self.obj) or None,


Should geometry be mandatory when exporting a grid property? E.g. raise if geometry is not given?

That will break current behaviour

Yes, good point, that would be a breaking change. Could be that we should nudge users towards using the geometry argument with a warning or similar? Or, rephrased: Will we aim for making this mandatory in the future?

jcrivenaes · 2024-04-25T11:10:19Z

Readability of the datamodel: For someone without prior knowledge, data.geometry could potentially be understood as "the geometry of these data". Aka similar to data.spec. Perhaps it should be data.relations.geometry or similar? (or possibly just relations.geometry on root level, next to data?)

Yes adding a "relations" on top level could be an option; this can possibly expand in the future, cf previous discussion. It might be that the current parent could also be added there. Let's get some input from others...

perolavsvendsen · 2024-04-25T11:18:51Z

Ref relative_path - the other relative_path (file.relative_path) is relative to the case, while the geometry.relative_path is relative to the runpath. Will this cause problems? Can we imagine grid data being exported outside the realization context, e.g. as a pre- or post-process?

Do we assume that clients will implement logic that looks for both fmu.realization.uuid AND geometry.relative_path?

An option could be to abstract this into a uuid instead, e.g. geometry.uuid. Then create that uuid as a hash of file.relative_path (when running outside FMU context) and fmu.case.uuid + file.relative_path (when inside FMU context). That way, clients will establish a pattern where they relate only to the uuid and we are free to refine how that uuid is made without breaking contract with clients.

Some mockup queries would probably be useful here, to see which patterns make sense to use, and to see what we are expecting (all!) consumers to intuitively understand.

mferrera · 2024-04-29T05:36:02Z

src/fmu/dataio/_utils.py

@@ -463,3 +463,35 @@ def glue_metadata_preprocessed(
    meta["tracklog"].extend(newmeta["tracklog"])

    return meta
+
+
+def get_geometry_ref(geometrypath: str | None, obj: Any) -> dict[str, str]:


Would it be reasonable for this to return an instance of Geometry Pydantic object instead?

tnatt · 2024-05-07T07:18:05Z

How does this geometry argument work together with the parent argument btw?

fmu-dataio/src/fmu/dataio/dataio.py

Lines 247 to 248 in 6545af5

    
                   parent: Optional. This key is required for datatype GridProperty, and 
        
                       refers to the name of the grid geometry.

The parent argument is only used to create the filename, but we could use the grid metadata data.name to set the parent string to be used in the filename if the geometry argument is present 🙂 then the users don't have to specify both!
Also if possible we should deprecate this parent argument, or at least reprase the doc wording required for datatype GridProperty and make it more clear what this parent argument is used for.

tnatt · 2024-05-07T07:29:22Z

I like the look of this PR, the geometry term is more familiar to users than parent (and in line with xtgeo wording 👍). We should make sure to documnet well how to use it with an example, maybe inside the documentation for the geometry argument.

Another thought, to me this geometry only relates to gridproperties, I don't think we would like to lock an attribute surface to a surface geometry. In e.g. Webviz as a user I would like to be able to drape my attribute surfaces onto any surface with time/depth units, not just one single.

perolavsvendsen · 2024-05-07T07:46:35Z

(...) the geometry term is more familiar to users than parent (and in line with xtgeo wording 👍).

In my mind, parent is different than geometry. While parent is a relationship (leaning towards data lineage, etc), geometry is more like "another piece of the same data" (-ish).

jcrivenaes · 2024-06-13T15:01:29Z

Redone in #677. Closing this

ENH: add data.geometry for option to link to geometry

40a9e64

This is particulary useful for a GridProperty, so we can refer to the grid object. But it can also be used for e.g. surfaces when wanting to link e.g. a property to a depth surface, for "draping" in WebViz.

jcrivenaes requested review from tnatt, perolavsvendsen and daniel-sol April 25, 2024 10:54

fixup: mypy linting

c593811

perolavsvendsen reviewed Apr 25, 2024

View reviewed changes

mferrera reviewed Apr 29, 2024

View reviewed changes

jcrivenaes mentioned this pull request Jun 12, 2024

ENH: Add optional geometry for Grid property #677

Merged

jcrivenaes closed this Jun 13, 2024

perolavsvendsen mentioned this pull request Jun 26, 2024

ENH: data.geometry tag is a file path, makes it difficult to find data #710

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add data.geometry for option to link to geometry #635

ENH: add data.geometry for option to link to geometry #635

jcrivenaes commented Apr 25, 2024 •

edited

Loading

perolavsvendsen commented Apr 25, 2024 •

edited

Loading

perolavsvendsen Apr 25, 2024

perolavsvendsen Apr 25, 2024

perolavsvendsen Apr 25, 2024

perolavsvendsen Apr 25, 2024

jcrivenaes Apr 25, 2024

perolavsvendsen Apr 25, 2024

jcrivenaes commented Apr 25, 2024

perolavsvendsen commented Apr 25, 2024 •

edited

Loading

mferrera Apr 29, 2024

tnatt commented May 7, 2024

tnatt commented May 7, 2024

perolavsvendsen commented May 7, 2024

jcrivenaes commented Jun 13, 2024

ENH: add data.geometry for option to link to geometry #635

ENH: add data.geometry for option to link to geometry #635

Conversation

jcrivenaes commented Apr 25, 2024 • edited Loading

perolavsvendsen commented Apr 25, 2024 • edited Loading

perolavsvendsen Apr 25, 2024

Choose a reason for hiding this comment

perolavsvendsen Apr 25, 2024

Choose a reason for hiding this comment

perolavsvendsen Apr 25, 2024

Choose a reason for hiding this comment

perolavsvendsen Apr 25, 2024

Choose a reason for hiding this comment

jcrivenaes Apr 25, 2024

Choose a reason for hiding this comment

perolavsvendsen Apr 25, 2024

Choose a reason for hiding this comment

jcrivenaes commented Apr 25, 2024

perolavsvendsen commented Apr 25, 2024 • edited Loading

mferrera Apr 29, 2024

Choose a reason for hiding this comment

tnatt commented May 7, 2024

tnatt commented May 7, 2024

perolavsvendsen commented May 7, 2024

jcrivenaes commented Jun 13, 2024

jcrivenaes commented Apr 25, 2024 •

edited

Loading

perolavsvendsen commented Apr 25, 2024 •

edited

Loading

perolavsvendsen commented Apr 25, 2024 •

edited

Loading