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 decaying D8 flow accumulation #389

Merged

Conversation

phargogh
Copy link
Member

@phargogh phargogh commented Apr 10, 2024

This PR copies the modifications to D8 flow accumulation implemented for the NCI Water Quality project that allow for pixel loads to decay as they pass over the landscape. You could think of like a pollutant flowing across the landscape, where some fraction is absorbed due to landcover or some other factor.

Tests have been implemented and runtime should only trivially be affected when decaying accumulation is not in use. When decaying accumulation is used, there is an extra cost in memory and in iteration over the pollutant contributions to the pixel.

Fixes #386

…/386-add-decaying-flow-accumulation

Conflicts:
	HISTORY.rst
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Hey @phargogh, sorry for the LONG delay here. This looks solid to me, I just had a few questions related to consistency.

Comment on lines 141 to 145
cdef struct DecayingValue:
double decayed_value # The value, which will be progressively updated as it decays
double min_value # The minimum value before the Decaying Value should be ignored.

cdef struct WeightedFlowPixelType:
Copy link
Member

Choose a reason for hiding this comment

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

I noticed the other struct definitions have a few comments above to describe them. Should these follow that same convention?

Copy link
Member Author

@phargogh phargogh Jun 21, 2024

Choose a reason for hiding this comment

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

Yep, great catch! Patched in 4f0e062

Comment on lines 146 to 147
int xi
int yi
Copy link
Member

Choose a reason for hiding this comment

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

The other struct definitions use unsigned int for pixel location, any reason not to do that here as well for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! In fact they really should be unsigned ints for consistency with the way indexes are used elsewhere in the function. Patched in d645b70

Comment on lines +1646 to +1648
# Search the block for root pixels.
# A root pixel is a flow direction pixel that is nodata, which means it
# may be a drain just off the edge.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

double decayed_value # The value, which will be progressively updated as it decays
double min_value # The minimum value before the Decaying Value should be ignored.

cdef struct WeightedFlowPixelType:
Copy link
Member

Choose a reason for hiding this comment

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

I think that this replaces ALL instances of FlowPixelType. Should we remove that struct definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, I'm still seeing FlowPixelType used in flow_accumulation_mfd, so I think it makes sense to leave it here for now.

@phargogh phargogh requested a review from dcdenu4 June 21, 2024 23:18
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dcdenu4 dcdenu4 merged commit 5e9fc84 into natcap:main Jun 24, 2024
90 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.

Contribute Pollutant Decay to Flow Accumulation
2 participants