Make fillsinks memory-order agnostic #125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes the comparison using array_equal and also checks to make sure that the memory layout was not changed by fillsinks.
This now passes the test that ensures that the memory order does not change within fillsinks, but it will fail the test that compares the two arrays.
This should now work if the input array is row or column major.
wkearn
added a commit
to wkearn/pytopotoolbox
that referenced
this pull request
Jan 16, 2025
The changes in TopoToolbox#125 use opensimplex directly in the tests, and I wanted to be clear that you need opensimplex to run the tests. I left the "opensimplex" optional dependency in there to avoid breaking workflows and because it does provide functionality that we use outside of the tests. I also added pytest to the "test" dependency. I believe this is the only other package we use directly. All of the other test dependencies are transitive dependencies. Cleanly installing everything using `pip install .[test]` in a new virtual environment works, and the tests are able to run successfully.
wkearn
added a commit
that referenced
this pull request
Jan 16, 2025
The changes in #125 use opensimplex directly in the tests, and I wanted to be clear that you need opensimplex to run the tests. I left the "opensimplex" optional dependency in there to avoid breaking workflows and because it does provide functionality that we use outside of the tests. I also added pytest to the "test" dependency. I believe this is the only other package we use directly. All of the other test dependencies are transitive dependencies. Cleanly installing everything using `pip install .[test]` in a new virtual environment works, and the tests are able to run successfully.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This represents one way that we might make our interface with libtopotoolbox agnostic to the memory order of our underlying numpy arrays. I have added a property
dimstoGridObject, which reverses the order of the dimensions if the array is in row-major layout.GridObject.shapestill returns(rows, columns). If we passself.dimsinstead ofself.shapeto thefillsinkswrapper, we will now get the same, correct, answer if theGridObject.zarray is column- or row-major. There is also a test that confirms this behavior.I did notice some unexpected changes in performance after implementing this. I thought it would tend to speed
fillsinksup when using row-major data by avoiding the unnecessary copy to column major. However, this was not the case when applyingfillsinksto the Big Tujunga data, loaded in row-major format. It takes around 25 ms on my machine to runfillsinkson the column-major data and 33 ms on the row-major data.I believe that this is not a problem with our implementation, but instead dependent on the data.
fillsinksis a global computation, and the amount of work it has to do depends on the orientation of features in the image. The Taiwan DEM performs better in row-major layout, and if I transpose the Big Tujunga DEM, the performance numbers swap: row-major takes around 33 ms and column-major takes around 25 ms.