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

Implement acv #100

Open
wkearn opened this issue Sep 2, 2024 · 3 comments · May be fixed by #143
Open

Implement acv #100

wkearn opened this issue Sep 2, 2024 · 3 comments · May be fixed by #143
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@wkearn
Copy link
Contributor

wkearn commented Sep 2, 2024

Implement the anisotropic coefficient of variation from GRIDobj.acv.

@wkearn wkearn added enhancement New feature or request good first issue Good for newcomers labels Sep 2, 2024
@Teschl
Copy link
Contributor

Teschl commented Sep 9, 2024

I am working on this now. This may be another good candidate for using OpenMP.

@Teschl
Copy link
Contributor

Teschl commented Oct 18, 2024

How should we go about testing the function? @wkearn

While testing on the randomly generated DEM will work, it won't really prove the function is working as intended. To implement the test, I would have to copy-paste the function into the test, so of course the result would be that all is correct. (Much like gradient8 by the way, this may need to be reworked, right?)

Can we create property-based tests for this? With my limited knowledge about the actual use of this function, can't think of a way to test this apart from creating a correct solution and verifying that the function reproduces it.

@wkearn
Copy link
Contributor Author

wkearn commented Oct 21, 2024

The property-based tests may not be the best way to test these sorts of functions. Good properties to test are more general than the implementation, such as testing that fillsinks actually does fill all the sinks.

Two alternatives come to mind:

  1. We could add a test executable that creates DEMs with fixed patterns. I.e. rather than random elevations, they have a constant slope or a dome or something. Then we can compute the solutions to gradient8 or acv or other similar functions analytically and compare them to the implemented functions. This is probably the easiest to set up within our existing testing framework, but it does require some effort to figure out good test cases and the analytical solutions.
  2. We could use snapshot tests to compare libtopotoolbox implementations to those output by TopoToolbox v2. I have some of that infrastructure set up in TopoToolbox/snapshot_data and TopoToolbox/topotoolbox3, but it requires some additional support in this repository (Implement snapshot tests #96). I have been working on getting that set up in libtopotoolbox, and I will try to push that forward some more this week.

The easiest way to use the snapshot testing infrastructure at the moment would probably be to implement things in a branch of libtopotoolbox, point your pytopotoolbox CMakeLists.txt to that branch and then run the comparisons from Python (because things like data loading and comparison of DEMs are readily available in Python). Then at least you could gain some confidence that your implementation works, even if we don't have the same automated tests set up here. You would also have to get snapshots for gradient8 and acv in the TopoToolbox/snapshot_data repository. The process for that is somewhat involved and not super well documented, though, so I can add those if you want.

@Teschl Teschl linked a pull request Oct 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants