Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Excesstopography #54

Merged
merged 7 commits into from
Jul 31, 2024
Merged

Add Excesstopography #54

merged 7 commits into from
Jul 31, 2024

Conversation

Teschl
Copy link
Contributor

@Teschl Teschl commented Jul 26, 2024

In this pull request, the excesstopography function is added to the topotoolbox. I also added some missing type hints to other functions.

Functionality:

  • supports single floats/ints, ndarrays or GridObjects as arguments for threshold_slopes.
  • both fsm2d and fmm2d are available
  • will automatically convert arrays/GridObjects to use right datatype/order
  • an example notebook has been added to the example gallery

closes #42

Missing: (for later pull requests)

  • have only added one test to this function as of right now. I think we should use one of our future meetings to discuss what we want to test exactly (in each function). It's very expensive to run the same test that are run for the libtopotoolbox again in python.
  • like we discussed, fmm3d is not implemented
  • All of this is not supporting the new dims in Pass dims to functions libtopotoolbox#69 and Pass dimensions as an array #53

@Teschl
Copy link
Contributor Author

Teschl commented Jul 26, 2024

It's very expensive to run the same test that are run for the libtopotoolbox again in python.

To add to that, the windows CI pipeline that builds and test already takes 2 minutes on Windows. I can't say how much of that is building, but I assume adding more (expensive) tests will increase the needed time a lot.
I don't know what duration is to be expected, but it does take drastically longer than mac/Ubuntu.

@wkearn
Copy link
Contributor

wkearn commented Jul 26, 2024

To add to that, the windows CI pipeline that builds and test already takes 2 minutes on Windows. I can't say how much of that is building, but I assume adding more (expensive) tests will increase the needed time a lot. I don't know what duration is to be expected, but it does take drastically longer than mac/Ubuntu.

The CI time is because libtopotoolbox takes forever to build on Windows, which I have also noticed in the libtopotoolbox CI. I don't really know what's going on there: it seems like it spends most of the time configuring the build rather than compiling the library. I'll open an issue over there to remind me to look into it more closely

TopoToolbox/libtopotoolbox/issues/71

@wkearn
Copy link
Contributor

wkearn commented Jul 26, 2024

* have only added one test to this function as of right now. I think we should use one of our future meetings to discuss what we want to test exactly (in each function). It's very expensive to run the same test that are run for the libtopotoolbox again in python.

I'm with you on not retesting all of the libtopotoolbox functionality: #55

@wkearn wkearn merged commit bf94d11 into TopoToolbox:main Jul 31, 2024
6 checks passed
@Teschl Teschl deleted the excesstopography branch July 31, 2024 20:46
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 excesstopography function
2 participants