Adds Graphflood binding

[bgailleton]

Binds graphflood (see this pull request ) to pytopotoolbox

[bgailleton]

Hi @wkearn , I am back to work and updated that branch to upstream.
I will still add a couple of stuff to it, but apart from that What do I still need to change to merge?

[wkearn]

@bgailleton: I think we added lints with flake8 after you made this PR (#59), so you'll need to fix those or let me know if we should change our lint settings. The other errors seem to be about data that you access locally in the tests. If you need those, we'll need to provide some way to access them. Our TopoToolbox/DEMs repository is where we put most of our test data. I am also trying to work out a good solution for storing test results that we can use for snapshot and regression testing (#55 and TopoToolbox/libtopotoolbox#96).

[wkearn]

Did you want test_graphflood.py to run as a test? pytest picks it up because it starts with test_. If we rename it the CI errors should go away,

If that script is meant to be documentation, we can turn it into a Jupyter notebook and include it with our other example notebooks, and either upload your Green River example to our DEM repository or use one of the DEMs we already have.

[bgailleton]

OK, thanks for the quick reply.
I'll take care of flake8 formatting (it's similar to clang-format right?). As of the test, it was more a local demo than a test, I'll rename it. I want to work on a proper tuto/test at some points, however my own tools already depends on a pytopotoolbox including graphflood and this PR. Once I sort these points would that be possible to accept the PR and I'll add tests/tuto in a future one?
Also is numpy<2 accepted? I still have a couple of tools non-compatible with numpy>=2 - although it should be fixed soon.

[wkearn]

OK, thanks for the quick reply. I'll take care of flake8 formatting (it's similar to clang-format right?).

Yes, if you run flake8 src/topotoolbox from the root directory of the repository it will print out the lint errors. You would need to make sure flake8 is installed, though. It's not included in our pyproject.toml.

As of the test, it was more a local demo than a test, I'll rename it. I want to work on a proper tuto/test at some points, however my own tools already depends on a pytopotoolbox including graphflood and this PR. Once I sort these points would that be possible to accept the PR and I'll add tests/tuto in a future one?

It is fine to just rename that file for now. If you want, once I merge this, I can turn it into a Jupyter notebook just so we have something, and you can add or change it later.

Also is numpy<2 accepted? I still have a couple of tools non-compatible with numpy>=2 - although it should be fixed soon.

This is a good question, which I haven't thought enough about. Generally we'd like to support as wide a range of dependency versions as possible, but we do depend on the NumPy C ABI through pybind11, which could limit things. I'll do some research and try to figure out exactly what we currently support.

[wkearn]

NumPy 2.0 is an ABI-breaking release, however it does contain support for building wheels that work on both 2.0 and 1.xx releases. It’s important to understand that:

When you build wheels for your package using a NumPy 1.xx version at build time, those will not work with NumPy 2.0.

When you build wheels for your package using a NumPy 2.x version at build time, those will work with NumPy 1.xx.

From https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-abi-handling

I've tested this, and it seems like as long as you build with numpy>=2.0.0, you can use older versions of numpy at run time.

The following patch works for fixing the build time dependency on numpy:

 pyproject.toml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/pyproject.toml b/pyproject.toml
index 2af7675..cc77510 100755
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,11 @@
 [build-system]
-requires = ["scikit-build-core","pybind11"]
 build-backend = "scikit_build_core.build"
+requires = [
+    "scikit-build-core",
+    "pybind11",
+    "numpy>=2.0.0"
+]
+

 [project]
 name = "topotoolbox"

I will go ahead and make that change. Let me know, @bgailleton, if you encounter any problems with your package versions after I do so.

See #66. That change shouldn't cause any merge conflicts here.

[bgailleton]

Should be ready to merge. I deleted the tests and will better examples later. I added a wrapper to run graphflood from a gridobj.
(I mis-clicked on close, you can ignore)

[wkearn]

Since this depends on the libtopotoolbox patch merged earlier this week (TopoToolbox/libtopotoolbox#134), I will wait until the next weekly release of libtopotoolbox on Monday (2024-W40) to merge. At that point we can bump the tag in CMakeLists.txt and run the checks one more time to make sure everything still works. The only reason it would not is if we merge #63 or #65 before then, and they conflict with the libtopotoolbox dependency, which should be unlikely.

[bgailleton]

OK with me!

[wkearn]

I just bumped the git tag and pushed that commit to your branch. I'll merge now, but you might want to check and make sure that everything still works on your end, since we don't yet have automated checks of graphflood in pytopotoolbox.

Further PRs are, of course, welcome. You should be considered a "contributor" by GitHub now, so the CI will run on automatically on your pull requests.