feat: Python interfaces to geos from Makutu repo (refactoring) #74

alexbenedicto · 2025-04-03T22:19:38Z

Closes #52

The goal of this PR, following #47 , was to refactor the job done in this first PR #44 by Makutu development team and @av-novikov. This refactor was made by focusing on 3 main axis:

Unify with functionalities developed in mesh_doctor
Use vtk library implemented methods when available
Clean up every method to work in general cases

For this 3rd point, efforts have been done to also keep certain methods that would work in certain workflows provided by Makutu development team, but by explicitly telling that these methods where limited to a certain scope of simulations.
More general methods have been added to work in different contexts and to complement these first methods.

I would like to thank @av-novikov , @bd713 , @sframba and @Victor-M-Gomes for their help through this work.

…ring

…into origin/feature/benedicto/rework_makutu_pygeos_tools

…on < 3.9

…ng the Solver classes

…lder

…cto/rework_makutu_pygeos_tools

alexbenedicto · 2025-04-15T21:21:26Z

There seems to be an issue with the setup-python action in the CI. I did not change the Github action between #71 and the merge into origin/main commit c3edf00 today.

Looking at actions/runner-images#11101, it looks like it occurs on the same date as deprecation was announced. I will wait and rerun the jobs later to see if it changes the Cache error found in the CI.

If that does not resolve the issue, maybe upgrade the version of actions/setup-python@v3 to match what is recommended in https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#caching-packages.

Any thoughts @mlemayTTE , @untereiner ?

paloma-martinez · 2025-04-16T12:44:38Z

@alexbenedicto I upgraded the checkout and setup-python github actions in PR #82, it should be fine once these modifications are merged

…cto/rework_makutu_pygeos_tools

…amples outside of src + update docs

mlemayTTE

What does obl stand for in pygeos-tools/examples/obl?

Looks OK for me. Just a few suggestions.

mlemayTTE · 2025-04-17T07:31:17Z

docs/pygeos_tools_docs/Example/reservoir.rst

Since the doc for pygeos-examples is generic and is not related to a specific python file, I am not sure having pygeos_tools_docs /Example/reservoir.rst is necessary. Maybe a example.rst file in pygeos_tools_docs is ok.

mlemayTTE · 2025-04-17T08:10:57Z

pygeos-tools/examples/obl/carbonated_water/phreeqc_dissolution/conversions.py

In the future, we may create unit converters in geos.utils using existing libraries like Pint

av-novikov · 2025-04-17T09:21:29Z

What does obl stand for in pygeos-tools/examples/obl?

@mlemayTTE, OBL stands for Operator-Based Linearization, these examples are working with ReactiveCompositionalMultiphaseOBL solver in GEOS. Let me know if/how i should rename the folder.

mlemayTTE · 2025-04-17T09:31:46Z

@av-novikov ok, thanks. Then, obl is another example of solver use.
It is not very important and I may have misunderstand what Example folder contains, but maybe the subfolder "solvers" is not necessary, files may be put directly in Example folder. And obl folder may be renamed reactiveCompositionalMultiphase_modeling, similarly to other examples.
Just a suggestion.

av-novikov

I checked my examples, they are working. Rest looks good to me.
Thanks @alexbenedicto !

av-novikov · 2025-04-17T10:27:32Z

pygeos-tools/examples/solvers/reservoir_modeling.py

+    time: float = 0.0
+    cycle: int = 0
+
+    solver.outputVtk( time )


Here and in other examples, for time=0 we are running solver.outputVtk(time) twice, before the loop and at first iteration as time is incremented after the call. For these two calls, the dt argument hidden inside is different, but not sure if it affects and resulting output is different. Anyway, running the output with the same time twice might be confusing.
@Victor-M-Gomes could you please check?

You're right, just taking out this line should be enough to avoid this. I am not very familiar with this example, so I couldn't spot the redundancy.

CusiniM · 2025-04-18T21:16:19Z

The merging of this PR had broken something on the GEOS CI. I would suggest that whenever you do bigger refactoring like this one, you also open a small geos PR just to make sure everything still works there.

Alternative we can stop pointing to the latest commit on main by default but it means that whenever you want to update the version used in geos you will have to update the commit hash somewhere in there.

CusiniM · 2025-04-18T21:45:33Z

pygeos-tools/pyproject.toml

+    "geos-utils @ file:./geos-utils",
+    "geos-mesh @ file:./geos-mesh",


this relative path is an issue.

untereiner · 2025-04-19T07:04:56Z

A clean decoupling would be first publishing these packages to pypi and then use packages from pypi inside the ci of geos.

mlemayTTE · 2025-04-22T08:48:12Z

I confess that relative paths are not very robust. This is the only way I found to make internal dependencies between geos Python packages without publishing them on PyPi. @untereiner do know a better solution allowing to build the packages locally without side effects on GEOS repo?

It is hard to predict that modifications of this repo will have side effects on GEOS repo. So at least for geos-ats and pygeos-tools that are directly linked to GEOS repo, a small geos PR just to make sure everything still works there is a good idea.

I agree with @untereiner, GEOS should be based on published Python packages to ensure a better stability.

untereiner · 2025-04-22T12:29:17Z

build the packages locally without side effects on GEOS repo?

no I don't.
I think the way to go is publishing to pypi and then dependabot will open a PR on GEOS repo that launches the CI that would eventually fail.
Maybe do changes to the behavior of a function or the public API of the wrapper only with caution :) Or write unit tests with the expected behavior from the GEOS side for these repos

alexbenedicto and others added 30 commits December 23, 2024 16:52

Created utils repository and updated every import for mesh_doctor

7f9fa60

Messages correction in io

c7bc10a

All the initial files from the Makutu PR are added for future refacto…

1948f60

…ring

Refactoring mesh files

50c6f02

Moved solverutils to utils

7fa1d04

Modified model directory structure + renamed vtkUtils into pyevtk_tools

7cae0cd

Adding acoustic / elastic modeling tools

b3a8637

Merge branch 'main' of https://github.com/GEOS-DEV/geosPythonPackages …

6735025

…into origin/feature/benedicto/rework_makutu_pygeos_tools

Correction exports and module names for pygeos-tools

9bd7c4d

Change imports for geos-mesh

3bbfbd7

Support processing of included XML files

5972c30

Fix to wave prop modeling in p4 and p3. Also fix to allow use of pyth…

cb666e4

…on < 3.9

Bug fix for io with VtkOutput

6ab34ef

Format update io and GeosxArgs

d4a08bb

Error handling for class methods

5990979

Update XML class

e1110db

Update Solver base class with also the use of wrapper file

0166a7f

Update WaveSolver

9530ffc

Update AcousticSolver

7ea90d2

bug fix

bba1762

Add minTime and maxTime handling

14ae4cb

Update ElasticSolver

55f6804

Update ReservoirSolver

bf3a827

Update GeomechanicsSolver

512156e

Update hinting of variables and methods

bf3be8e

Add examples of scripts to pilot a GEOS simulation through pygeos usi…

ccb6b57

…ng the Solver classes

Add __doc__ at the beginning of scritps

c8fa6dc

Moved examples inside utilities

0112ad4

Change examples folder to solvers-examples

2ec5997

Moved all files outside of "utilities" folder and remove utilities fo…

176b03f

…lder

alexbenedicto and others added 5 commits April 11, 2025 15:22

Move checking of xml file from __init__ to initialize method

0c5959b

Add examples for ReactiveCompositionalMultiphaseOBL solver

95a4dff

Merge remote-tracking branch 'origin/main' into origin/feature/benedi…

c3edf00

…cto/rework_makutu_pygeos_tools

Upgrade python version to 3.10

940e27e

Remove examples to check if CI cache build error still occurs

3c26d90

alexbenedicto added 2 commits April 16, 2025 09:22

Merge remote-tracking branch 'origin/main' into origin/feature/benedi…

a79a9b8

…cto/rework_makutu_pygeos_tools

Readded the examples for obl provided by Aleks Novikov + moved all ex…

4306dce

…amples outside of src + update docs

alexbenedicto requested review from av-novikov, Victor-M-Gomes, mlemayTTE and paloma-martinez April 16, 2025 20:12

mlemayTTE approved these changes Apr 17, 2025

View reviewed changes

av-novikov added 3 commits April 17, 2025 11:35

Fix name of the *.xml in 2ph_comp example

d05f90b

Fix timestepping in OBL solver examples

7950e43

Rename folder with OBL examples

a62cf92

av-novikov approved these changes Apr 17, 2025

View reviewed changes

av-novikov mentioned this pull request Apr 17, 2025

feat: Adaptive multilinear interpolation for OBL solver GEOS-DEV/GEOS#3391

Open

11 tasks

alexbenedicto added 2 commits April 17, 2025 10:33

Updated example.rst and yapf formatting

d4db9e6

Fix write_mesh

5d5ce74

alexbenedicto merged commit 5a0f75e into main Apr 17, 2025
39 checks passed

alexbenedicto deleted the origin/feature/benedicto/rework_makutu_pygeos_tools branch April 17, 2025 20:21

CusiniM reviewed Apr 18, 2025

View reviewed changes

		"geos-utils @ file:./geos-utils",
		"geos-mesh @ file:./geos-mesh",

feat: Python interfaces to geos from Makutu repo (refactoring) #74

feat: Python interfaces to geos from Makutu repo (refactoring) #74

Uh oh!

Conversation

alexbenedicto commented Apr 3, 2025

Uh oh!

alexbenedicto commented Apr 15, 2025

Uh oh!

paloma-martinez commented Apr 16, 2025

Uh oh!

mlemayTTE left a comment

Choose a reason for hiding this comment

Uh oh!

mlemayTTE Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

mlemayTTE Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

av-novikov commented Apr 17, 2025

Uh oh!

mlemayTTE commented Apr 17, 2025

Uh oh!

av-novikov left a comment

Choose a reason for hiding this comment

Uh oh!

av-novikov Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Victor-M-Gomes Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CusiniM commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CusiniM Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

untereiner commented Apr 19, 2025

Uh oh!

mlemayTTE commented Apr 22, 2025

Uh oh!

untereiner commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CusiniM commented Apr 18, 2025 •

edited

Loading

untereiner commented Apr 22, 2025 •

edited

Loading