Skip to content
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 simplified export for depth prediction surfaces #1033

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

tnatt
Copy link
Collaborator

@tnatt tnatt commented Mar 1, 2025

Resolves #1045

PR to add a new standard_result: structure_depth_surface, and a simple export function export_structure_depth_surfaces to be used to export depth horizons from within a horizon folder in RMS.

Addresses https://github.com/equinor/atlas/issues/42

@tnatt tnatt force-pushed the simple-export-depth-surfaces branch 2 times, most recently from d96e2e8 to 617b7ec Compare March 2, 2025 18:05
@tnatt tnatt marked this pull request as ready for review March 4, 2025 12:02

## Result

The surfaces from within the horizon folder will be exported as 'irap_binary'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to state this explicitly? Or, rather, do we want to be able to change this? Currently, it sort of looks like an invitation to a discussion around which folder and filenames that should be used - but ideally nobody is going to relate to that.



@experimental
def export_depth_prediction_surfaces(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export_structure_depth_predictions?
export_structure_depth_prediction_surfaces?

Seen in relations to:
export_structure_thickness_predictions?

Or just drop the whole "prediction" from the name (implying that this is just "given")?
export_structure_depth_surfaces?
export_structure_thickness_surfaces?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(export_)structure_depth_surfaces
(export_)structure_thickness_surfaces
(export_)structure_depth_fault_lines
(export_)structure_depth_fault_surfaces
(export_)structure_pinchout_lines
(export_)structure_xx
(export_)structure_yy

(export_)grid_depth_surfaces
(export_)grid_thickness_surfaces
(export_)grid_parameters
(export_)grid_qc

...
...
...

@perolavsvendsen
Copy link
Member

Ref discussions offline. I think good requirements for this could include:

  • Increase robustness + be implementable: Minimum footprint in FMU workflows
  • Increase robustness: Changeable without disruption for end users
  • Increase data quality: Provide consistency across all FMU workflows, and enable other applications to confidently consume The Prediction Of The Structural Model for any FMU run.
  • Follow established conventions on naming/establishes naming conventions that scale.
  • Intuitive for end users
  • Documented
  • ...
  • ...

Probably smart to see this in relations to other similar export functions in terms of naming and the implicit conventions that are starting to form. E.g. what would one-liners look like for this list of identified items? Do they make sense when they are all combined in the same documentation? (Would it be worth trying to do that as an acid test for the naming?)

@tnatt tnatt force-pushed the simple-export-depth-surfaces branch 7 times, most recently from bb9441c to d2383ed Compare March 5, 2025 15:07
@tnatt tnatt force-pushed the simple-export-depth-surfaces branch from d2383ed to 87ac7aa Compare March 5, 2025 21:50
@tnatt tnatt self-assigned this Mar 5, 2025
@tnatt tnatt force-pushed the simple-export-depth-surfaces branch from 87ac7aa to c4f6eb8 Compare March 5, 2025 22:02
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job 👍 have you checked a documentation build visually?

content="depth",
unit=self._unit,
vertical_domain="depth",
domain_reference="msl",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always be relative to msl? (Just a domain curiosity from my end)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conventionally yes, but possibly no, not always 🤔 Perhaps that assumption should be noted in the docs as well?

Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I tried this in RMS now and works like charm 👍

@perolavsvendsen
Copy link
Member

And we are (sufficiently) happy with the function name? I.e. will it be understood that you can only include this in your workflow exactly 1 time? And that there will be only 1 set of depth surfaces produced from it? That is - you must use this for the surfaces that are predictions but you cannot use it for all those surfaces that are just for QC etc.

(Since we have now taken away the term "prediction" from the method name, ref discussion yesterday)

@tnatt tnatt force-pushed the simple-export-depth-surfaces branch from c4f6eb8 to fccce4d Compare March 6, 2025 11:44
@tnatt
Copy link
Collaborator Author

tnatt commented Mar 6, 2025

And we are (sufficiently) happy with the function name? I.e. will it be understood that you can only include this in your workflow exactly 1 time? And that there will be only 1 set of depth surfaces produced from it? That is - you must use this for the surfaces that are predictions but you cannot use it for all those surfaces that are just for QC etc.

(Since we have now taken away the term "prediction" from the method name, ref discussion yesterday)

I think so! But I can stress test it with the RM3 group today, we have a technical meeting at 14 👍

@tnatt tnatt force-pushed the simple-export-depth-surfaces branch 2 times, most recently from 6dc53da to 304a40e Compare March 6, 2025 11:52
@tnatt tnatt force-pushed the simple-export-depth-surfaces branch from 304a40e to 1a6332b Compare March 6, 2025 20:58
@tnatt
Copy link
Collaborator Author

tnatt commented Mar 6, 2025

And we are (sufficiently) happy with the function name? I.e. will it be understood that you can only include this in your workflow exactly 1 time? And that there will be only 1 set of depth surfaces produced from it? That is - you must use this for the surfaces that are predictions but you cannot use it for all those surfaces that are just for QC etc.
(Since we have now taken away the term "prediction" from the method name, ref discussion yesterday)

I think so! But I can stress test it with the RM3 group today, we have a technical meeting at 14 👍

Thumbs up from the group of modellers 👍 I showed them the documentation and they all understood this to be their final surfaces.
Also it seems removing the word prediction was a good choice, the consensus was that the term was much more confusing and all were in favor of the given name 🙂

@tnatt tnatt merged commit 7a48f69 into equinor:main Mar 6, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add export_structure_depth_surfaces() simple exporter
4 participants