Skip to content

ENH: Add to_rms()/from_rms() methods in NestedHybridGrid - #490

Merged
tnatt merged 1 commit into
equinor:mainfrom
tnatt:add-to-from-rms-nested
Aug 18, 2026
Merged

ENH: Add to_rms()/from_rms() methods in NestedHybridGrid#490
tnatt merged 1 commit into
equinor:mainfrom
tnatt:add-to-from-rms-nested

Conversation

@tnatt

@tnatt tnatt commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Resolves #468

PR that simplifies the initialization of the NestedHybridGrid in RMS, by allowing users to enter names of objects inside RMS instead of xtgeo objects. It also enables easy writing of the grid and all of it's appended properties to RMS.

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 ✅

@tnatt
tnatt force-pushed the add-to-from-rms-nested branch 5 times, most recently from 27cb4ea to 581762c Compare August 12, 2026 14:27
@tnatt tnatt self-assigned this Aug 13, 2026
@tnatt
tnatt marked this pull request as ready for review August 13, 2026 10:38
@tnatt
tnatt requested review from alifbe and nourinmohd August 13, 2026 10:38

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.

_make_box_grid_with_region by default create region with id = 2, which means the test below should not create any nested grid. But since the assertion is only checking mock call counts/arguments, this didn't get caught.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the _make_box_grid_with_region creates a region property where half the values are 1 and half the values are 2... so we are creating a nested grid - just refining the other half than what the target_region_id is saying .. a bit confusing 😄 so will make it more clear and add more test that we do refine the area that has value 1

@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.

The new class is expecting region value = 1 for nested hybrid grid but the coarse grid is created with region value = 2. But none of the test capture this bug.

Suggest to change test strategy to check if nested grid is created.

@tnatt

tnatt commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

The new class is expecting region value = 1 for nested hybrid grid but the coarse grid is created with region value = 2. But none of the test capture this bug.

Suggest to change test strategy to check if nested grid is created.

Good catch 👀 will update!

@tnatt
tnatt force-pushed the add-to-from-rms-nested branch 7 times, most recently from 808464a to b8e19c8 Compare August 17, 2026 13:15
@tnatt
tnatt requested a review from alifbe August 17, 2026 13:18
@tnatt

tnatt commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

Have a look again @alifbe, I added more checks regarding the actual output itself - not just if expected calls are being made

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.

Not related, but this should be list[xtgeo.GridProperty] right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yup! will fix it while here

@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.

This looks great 💯

Just spot something unrelated to this PR. Not sure if it can be part of this PR or another PR.

@tnatt
tnatt force-pushed the add-to-from-rms-nested branch from b8e19c8 to 9f8773c Compare August 18, 2026 13:25
@tnatt
tnatt merged commit 1525128 into equinor:main Aug 18, 2026
10 checks passed
@tnatt
tnatt deleted the add-to-from-rms-nested branch August 18, 2026 13:31
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 from_rms() / to_rms() methods on NestedHybridGrid

2 participants