-
Notifications
You must be signed in to change notification settings - Fork 15
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
ENH: add data.geometry for option to link to geometry #635
ENH: add data.geometry for option to link to geometry #635
Conversation
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.
Readability of the datamodel: For someone without prior knowledge, |
relative_path: some_relative/path/geometry.roff | ||
|
||
This means that the geometry may be 'located' both on disk (relative path) and in | ||
SUMO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Sumo, not SUMO :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref section 8, paragraph 24 of the Sumo brand guidelines (revision 6): "Sumo is a name, not an abbreviation."
os.chdir(rmssetup) | ||
|
||
edata = dataio.ExportData( | ||
config=rmsglobalconfig, content="depth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should geometry be mandatory when exporting a grid property? E.g. raise if geometry is not given?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will break current behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
Ref Do we assume that clients will implement logic that looks for both An option could be to abstract this into a 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. |
@@ -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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be reasonable for this to return an instance of Geometry
Pydantic object instead?
How does this fmu-dataio/src/fmu/dataio/dataio.py Lines 247 to 248 in 6545af5
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.
|
I like the look of this PR, the 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. |
In my mind, |
Redone in #677. Closing this |
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
It will then create in the metadata: