Skip to content

Conversation

@hflesche
Copy link
Contributor

  • New models are added for pressure sensitivity, as well as an abstract base class and tests.
  • Optimisation utilities are moved to a separate module to avoid circular imports.

… add models for polynomial, friable, patchy cement; add tests
@hflesche hflesche requested a review from a team as a code owner October 15, 2025 14:33
@hflesche hflesche requested a review from Copilot October 15, 2025 14:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new family of pressure sensitivity models (polynomial plus friable and patchy cement dry bulk/shear models) with a shared abstract BasePressureModel, refactors existing exponential and sigmoidal models to use the common interface, and relocates optimisation utilities to a dedicated optimisation_utilities package to avoid circular imports. Also introduces extensive new test suites (unit, integration, performance/benchmark) and updates imports accordingly. Key API change in patchy_cement_model introduces a new patchy_cement_model_dry function while altering the return signature of patchy_cement_model_cem_frac.

  • Introduces BasePressureModel and new Polynomial, Friable, and PatchyCement pressure models
  • Relocates optimisation helpers to rock_physics_open.equinor_utilities.optimisation_utilities
  • Alters patchy_cement_model_cem_frac return values (potential breaking change) and adds large new test coverage including benchmarks

Reviewed Changes

Copilot reviewed 28 out of 30 changed files in this pull request and generated 22 comments.

Show a summary per file
File Description
tests/t_matrix_model_tests/test_t_matrix_optimisation.py Updated import path to new optimisation utilities module.
tests/t_matrix_model_tests/test_opt_param_to_ascii.py Updated import for opt_param_to_ascii to new module location.
tests/machine_learning_utilities_test/test_pressure_models_benchmark.py Added performance and memory benchmark tests for new/updated pressure models.
tests/machine_learning_utilities_test/test_pressure_models.py Comprehensive unit/integration tests for all pressure model classes.
tests/machine_learning_utilities_test/conftest.py Added fixtures, parametrization, and markers for pressure model testing.
src/rock_physics_open/t_matrix_models/* Refactored imports to new optimisation utilities location.
src/rock_physics_open/t_matrix_models/init.py Removed re-export of optimisation utilities (now relocated).
src/rock_physics_open/t_matrix_models/carbonate_pressure_substitution.py Adjusted run_regression import path.
src/rock_physics_open/sandstone_models/* Updated optimisation utility imports; added patchy_cement_model_dry export.
src/rock_physics_open/sandstone_models/patchy_cement_model.py Added patchy_cement_model_dry and changed patchy_cement_model_cem_frac return signature.
src/rock_physics_open/equinor_utilities/optimisation_utilities/init.py New package initializer exporting optimisation routines.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/base_pressure_model.py New abstract base class defining common interface & serialization.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/exponential_model.py Refactored to subclass BasePressureModel and simplified API.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/sigmoidal_model.py Rewritten as SigmoidalPressureModel based on BasePressureModel.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/polynomial_model.py New PolynomialPressureModel implementation.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/friable_pressure_models.py New friable bulk & shear pressure models.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/patchy_cement_pressure_models.py New patchy cement bulk & shear pressure models.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/import_ml_models.py Updated dynamic loader to recognize new model types.
src/rock_physics_open/equinor_utilities/machine_learning_utilities/init.py Updated public exports to new model classes.

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

@hflesche hflesche force-pushed the feat/polynomial-model branch 4 times, most recently from a2f477b to e53bb69 Compare October 16, 2025 06:10
Copy link
Contributor

@einarwar einarwar left a comment

Choose a reason for hiding this comment

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

Good job! Really nice move to create a common base class for the models, so there is less code duplication.

This review might seem nitpicky, but they are small changes that can make it easier for others to read and understand the code, as well as get more help from the IDE to avoid bugs :)

Comment on lines +115 to +120
return {
"a_factor": self._a_factor,
"b_factor": self._b_factor,
"model_max_pressure": self._model_max_pressure,
"description": self._description,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: To get more help from the editor, and make it more clear what keys and the value types one could define a typing.TypedDict definition for this dictionary.

E.g:

class ModelDict(TypedDict):
    a_factor: float
    b_factor: float
    model_max_pressure: float | None
    description: str

One could then use the ModelDict as a type annotation for the method.
This would however, conflict with the type annotation from the abstract method from the base class, as it is typed as a dict[str, Any]. which is incompatible with a TypedDict. One could however, change the abstract base class method to be of type collections.abc.Mapping[str, Any]

)
return inp_arr

def predict_abs(self, inp_arr: np.ndarray, case: str = "in_situ") -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: For the case argument, consider using the type typing.Literal["in_situ", "depleted"] to explicitly specify that the only valid string values is those two. Currently the behaviour is that it treats "depleted" and every other string as the same. So even if i entered "some_case" i would get the p_depleted.

In order to not break compatibility, one could simply issue a warning that in the future, one has to use the literal strings "in_situ" and "depleted", and inform the user that if the string was not "in_situ" it will be "depleted"

config.addinivalue_line("markers", "benchmark: marks performance benchmark tests")


def pytest_collection_modifyitems(config, items):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type annotations here:

Suggested change
def pytest_collection_modifyitems(config, items):
def pytest_collection_modifyitems(config: pytest.Config, items: Iterable[pytest.Item]):


# Parametrized fixtures for comprehensive testing
@pytest.fixture(params=["exponential_model", "polynomial_model", "sigmoidal_model"])
def simple_model(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type annotations

Suggested change
def simple_model(request):
def simple_model(request: pytest.FixtureRequest):

Applies for more fixtures below

Copy link
Contributor

Choose a reason for hiding this comment

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

I know its a common practice, and i think even the pytest docs do not type annotate their fixtures when used in tests. I think however there is a lot to gain from also annotating the type gotten from fixtures, as we can get IDE help with the available attributes and methods of the objects. Consider doing that for this file :)

@hflesche hflesche force-pushed the feat/polynomial-model branch from 7494c54 to 350ac82 Compare October 21, 2025 04:28
@hflesche hflesche merged commit 34ea21f into main Oct 21, 2025
13 checks passed
@hflesche hflesche deleted the feat/polynomial-model branch October 21, 2025 04:32
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.

2 participants