Skip to content

swfunction Add new method for height above FFL and refactoring - #300

Merged
alifbe merged 5 commits into
mainfrom
update-swtools
Sep 22, 2025
Merged

swfunction Add new method for height above FFL and refactoring#300
alifbe merged 5 commits into
mainfrom
update-swtools

Conversation

@alifbe

@alifbe alifbe commented Sep 18, 2025

Copy link
Copy Markdown
Contributor
  • Add new method for computing height above FFL (and removed invalid one)
  • Refactor debug mode and instead return useful properties together with Sw. As library, swfunction should let users handle the properties themselves. Note: This is breaking change.

@alifbe
alifbe requested a review from Copilot September 18, 2025 11:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new method for calculating height above free fluid level (FFL) called "truncated_cell_corners_above_ffl" and introduces a breaking change by refactoring the API to return multiple properties instead of just the saturation value.

Key changes:

  • Add support for "truncated_cell_corners_above_ffl" method in calculations
  • Change compute() method to return a tuple of 4 properties (SW, HTOP, HBOT, HCENTER) instead of just SW
  • Remove debug mode functionality and parameters

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/fmu/tools/properties/swfunction.py Updated API to return tuple of properties, removed debug functionality, added new method support
tests/properties/test_swfunction.py Updated test calls to use tuple indexing and added test cases for new method
docs/properties.rst Updated documentation example to use new tuple return format

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/fmu/tools/properties/swfunction.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/fmu/tools/properties/swfunction.py Outdated
Comment thread src/fmu/tools/properties/swfunction.py Outdated

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

Cf. comment on return a dict in stead of tuple; otherwise LGTM

@alifbe
alifbe requested a review from Copilot September 22, 2025 07:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/fmu/tools/properties/swfunction.py
Comment on lines +294 to +296
def compute(
self, compute_method: str = "integrated"
) -> dict[str, xtgeo.GridProperty]:

Copilot AI Sep 22, 2025

Copy link

Choose a reason for hiding this comment

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

This is a breaking change to the public API. The method now returns a dictionary instead of a single GridProperty. Consider documenting this breaking change more prominently or providing a deprecation path for existing users.

Copilot uses AI. Check for mistakes.
@alifbe
alifbe merged commit ab84ff8 into main Sep 22, 2025
9 checks passed
@alifbe
alifbe deleted the update-swtools branch September 22, 2025 07:52
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.

3 participants