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 FlowObject #61

Merged
merged 23 commits into from
Sep 2, 2024
Merged

Add FlowObject #61

merged 23 commits into from
Sep 2, 2024

Conversation

Teschl
Copy link
Contributor

@Teschl Teschl commented Aug 16, 2024

This pull request contains the following:

  1. Using dims[2] instead of nrows, ncols when using libtopotoolbox functions. Closes Pass dimensions as an array #53
  2. Added private methods gwdt_computecosts, gwdt, flow_routing_d8_carve and flow_routing_targets. Closes Add gwdt function  #45
  3. Added FlowObject with constructor. Closes Implement FlowObject #60

Limitations:

  1. At the moment, GIT_TAG main is used in the CMakeLists.txt since flow_routing_targets will only be added to libtopotoolbox next Monday. (Fixed with the new release)
  2. Arrays that are not needed anymore will be reused in the constructor of the FlowObject. There is the option to delete unused arrays as soon as possible using del var_name which could be beneficial when working with big DEMs.
  3. The private methods are not used in the constructor to reduce computational load, so I'm not sure if they are even needed.

Regarding the new FlowObject #60 :

  1. The FlowObject contains all variables of the GridObject it's made from. It is my understanding that using self.z = dem will result in the array being only stored once, and both variables (z in GridObject and FlowObject) will point to the same data.
  2. The show function will print the DEM, target and source array. That's probably not how we want it to work, but for now it's sort of a proof that something is actually being computed.
  3. Maybe the FlowObject should contain the source GridObject as a variable instead of copying each variable.

I have also added the new class to the documentation and added a basic example notebook which will be expanded later on. The private functions are not listed in the documentation, but still have doc strings.

Comment on lines +89 to +111
def show(self, cmap: str = 'terrain'):
"""
Display the FlowObject instance as an image using Matplotlib.

Parameters
----------
cmap : str, optional
Matplotlib colormap that will be used in the plot.
"""

fig, axs = plt.subplots(1, 3, figsize=(15, 5))

axs[0].imshow(self.z, cmap=cmap)
axs[0].set_title('dem')

axs[1].imshow(self.target, cmap=cmap)
axs[1].set_title('target')

axs[2].imshow(self.source, cmap=cmap)
axs[2].set_title('source')

plt.tight_layout()
plt.show()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now. TopoToolbox v3 doesn't really have a default plot method for FLOWobj. I think typically you create a STREAMobj, which is more interesting to visualize.

Comment on lines +78 to +87
# raster metadata
self.z = dem
self.target = target
self.source = source
self.shape = grid.shape

# georeference
self.bounds = grid.bounds
self.transform = grid.transform
self.crs = grid.crs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine to copy the GridObject metadata into the FlowObject. Maybe there is some application where it would be better to have a reference to the original GridObject, but we can change this easily enough if we figure that out.

I am also pretty sure that self.z = dem just copies a reference to the array and doesn't copy the array data, which I think is what we want. This does make things a little weird if you modify fd.z, since that modification will be applied to dem.z, but users generally shouldn't be modifying the DEM through the FlowObject, not least because it invalidates the flow directions.

@wkearn wkearn merged commit 6d1ec92 into TopoToolbox:main Sep 2, 2024
7 checks passed
@Teschl Teschl deleted the pass_args branch September 3, 2024 13:28
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.

Implement FlowObject Pass dimensions as an array Add gwdt function
2 participants