Skip to content

ENH: Add original I,J,K as properties in NestedHybridGrid - #492

Merged
nourinmohd merged 7 commits into
equinor:mainfrom
nourinmohd:470_add_ijk_properties
Sep 24, 2026
Merged

nourinmohd merged 7 commits into
equinor:mainfrom
nourinmohd:470_add_ijk_properties

Conversation

@nourinmohd

@nourinmohd nourinmohd commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Resolves #470

  • Adds preservation of original grid I/J/K coordinates for NestedHybridGrid.

  • _build_nested_hybrid_grid() now appends I_orig, J_orig, and K_orig properties to the coarse grid before refinement and merge, allowing xtgeo.grid_merge() to transfer them automatically to the final nested hybrid grid. These properties are available through nhg.properties and nhg.grid.get_prop_by_name(...).

  • Also updates the nested hybrid grid property test to verify the new properties are present and retain the original 1-based coordinates.

Checklist

  • Tests added (if not, comment why)
  • Test coverage equal or up from main (run pytest with --cov=src/ --cov-report term-missing)
  • If not squash merging, every commit passes tests
  • Appropriate commit prefix and precise commit message used
  • All debug prints and unnecessary comments removed
  • Docstrings are correct and updated
  • Documentation is updated, if necessary
  • Latest main rebased/merged into branch
  • Added comments on this PR where appropriate to help reviewers
  • Moved issue status on project board
  • Checked the boxes in this checklist ✅

@nourinmohd
nourinmohd requested a review from tnatt August 19, 2026 07:58
@nourinmohd

Copy link
Copy Markdown
Contributor Author
  • Add a constant for the I_org, J_org, and K_org instead of passing them as strings.

@nourinmohd
nourinmohd force-pushed the 470_add_ijk_properties branch from 499b863 to db7e53a Compare August 21, 2026 08:31
@nourinmohd
nourinmohd requested a review from alifbe September 22, 2026 10:10

@alifbe alifbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These properties is very useful for mapping between nested grid and parent grid which is hopefully can be fully automated from within NHG class. So, at least for now, we don't see the needs for this property to be exported to RMS. Please ensure that .to_rms() method doesn't export these IJK properties.

Comment thread src/fmu/tools/nestedhybridgrid/nestedhybrid.py Outdated
Comment thread tests/nestedhybridgrid/test_nestedhybrid.py Outdated
Comment thread tests/nestedhybridgrid/test_nestedhybrid.py Outdated
@nourinmohd
nourinmohd force-pushed the 470_add_ijk_properties branch from 450eed8 to 524b5a7 Compare September 23, 2026 07:02
@nourinmohd
nourinmohd requested a review from alifbe September 23, 2026 07:03
@nourinmohd

Copy link
Copy Markdown
Contributor Author

Implemented:

  • Renamed IJK properties to parent_I, parent_J, and parent_K.
  • .to_rms() now excludes these internal mapping properties.
  • Updated tests to use ORIGINAL_IJK_PROPERTY_NAMES.

@tnatt tnatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me 🙂
We might want to add some info regarding these properties into the documentation, to inform they are there.. but we can maybe wait and see if the need arises. If there is a need, the best solution would perhaps be to add a flag in the to_rms function to easily store these properties in rms.

@alifbe alifbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Please consider changing the commit message. For some case where user do nested over nested, original could be interpreted as origin grid. But in this case, the property that is created is direct parent.

@nourinmohd
nourinmohd merged commit 1b30482 into equinor:main Sep 24, 2026
10 checks passed
@nourinmohd
nourinmohd deleted the 470_add_ijk_properties branch September 24, 2026 07:21
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 original I,J,K as properties in the NestedHybridGrid

3 participants