-
Notifications
You must be signed in to change notification settings - Fork 76
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
Mutually mask SDR inputs to avoid problems routing #1435
Merged
dcdenu4
merged 10 commits into
natcap:main
from
phargogh:bugfix/1433-sdr-large-swaths-of-nodata
Nov 17, 2023
Merged
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d6cb761
Taking a first stab at implementing mutual masking.
phargogh 51eaed1
Reworking mutual masking to accommodate drainage.
phargogh 1caecc9
Updating regression values.
phargogh b23a6ea
Noting change in HISTORY. RE#1433
phargogh 6743ff2
Correcting RST syntax. RE:#1433
phargogh ac0df23
Merge branch 'main' of https://github.com/natcap/invest into bugfix/1…
phargogh 2af4a47
Updating SDR regression values where affected by mutual-masking.
phargogh f33cf70
Replacing another masking lambda with a named function. RE:#1433
phargogh 01073db
Merge branch 'main' of https://github.com/natcap/invest into bugfix/1…
phargogh ee7ea11
Separating mutual mask from single-raster mask ops.
phargogh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used
raster_map
much yet, but this is cool to see!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! Masking is such an easy thing to express within the context of
raster_map
, it was just too good to pass up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, using a
lambda
like this will break whenn_workers > 0
. I just discovered this issue while working on #1437. Because lambdas can't be pickled, they can't be directly passed into a taskgraph Task.op
has to be a named function defined at the top level of the module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, of course! Thanks for the reminder. Patched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phargogh should this be
_mask_op
now inkwargs
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I missed one lambda and patched it in f33cf70