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 trunk to StreamObject #153

Merged
merged 8 commits into from
Feb 14, 2025
Merged

Add trunk to StreamObject #153

merged 8 commits into from
Feb 14, 2025

Conversation

Teschl
Copy link
Contributor

@Teschl Teschl commented Feb 12, 2025

This implements the trunk function. I (finally) got the trunk function working, more or less copying the Matlab version directly but modifying it to work in the python version.

This still needs work:

  • The variable names are taken directly from the Matlab version. I'm not a big fan of these variable names, they should be more descriptive. (also, they don't conform to pep8 in some places, so a failing pipeline is expected)
  • It also doesn't support the use of arguments A (GRIDobj with flow accumulation values) and a (node-attribute list). Should we include this functionality (in parts or all of it)?

@wkearn Are there variable names that should be kept in the current form, or can I go ahead and replace them by more descriptive names? I assume some of them are more or less standard for what they represent.

closes #113

@Teschl Teschl marked this pull request as draft February 12, 2025 20:20
@wkearn
Copy link
Contributor

wkearn commented Feb 13, 2025

This implements the trunk function. I (finally) got the trunk function working, more or less copying the Matlab version directly but modifying it to work in the python version.

Looks good, @Teschl! I am curious if we could do something to avoid the sparse matrix manipulation, but it makes sense to have a working version based on the MATLAB implementation that we can experiment with in the future.

This still needs work:

* The variable names are taken directly from the Matlab version. I'm not a big fan of these variable names, they should be more descriptive. (also, they don't conform to pep8 in some places, so a failing pipeline is expected)

@wkearn Are there variable names that should be kept in the current form, or can I go ahead and replace them by more descriptive names? I assume some of them are more or less standard for what they represent.

I think we can replace these variable names with more descriptive ones. OUTLET is the only scientifically meaningful one, users won't see any of this anyway, and we might as well follow the standard. I might suggest OUTLET => outlets, II => max_neighbor and I => trunks. Feel free to use ones that make more sense to you if you can think of some.

* It also doesn't support the use of arguments `A` (GRIDobj with flow accumulation values) and  `a` (node-attribute list). Should we include this functionality (in parts or all of it)?

This shouldn't be too hard to add because we have StreamObject.ezgetnal. You should be able to do something like

def trunk(self, a=None):
  # ...
  if a is None:
    dds = self.downstream_distance()
  else:
    dds = self.ezgetnal(a)

ezgetnal will throw an error if a is not the right data type or size.

closes #113

@Teschl Teschl marked this pull request as ready for review February 13, 2025 16:42
@wkearn wkearn merged commit 7fa0ea3 into TopoToolbox:main Feb 14, 2025
7 checks passed
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 trunk
2 participants