Mutually mask SDR inputs to avoid problems routing #1435
Conversation
|
This solution is waiting on the next release of pygeoprocessing. |
src/natcap/invest/sdr/sdr.py
Outdated
| func=pygeoprocessing.raster_map, | ||
| kwargs={ | ||
| 'op': lambda array, mask: array, |
There was a problem hiding this comment.
I haven't used raster_map much yet, but this is cool to see!
There was a problem hiding this comment.
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.
Unfortunately, using a lambda like this will break when n_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.
Aha, of course! Thanks for the reminder. Patched.
There was a problem hiding this comment.
@phargogh should this be _mask_op now in kwargs?
There was a problem hiding this comment.
Oh yes, I missed one lambda and patched it in f33cf70
|
This is waiting on #1437 |
src/natcap/invest/sdr/sdr.py
Outdated
| func=pygeoprocessing.raster_map, | ||
| kwargs={ | ||
| 'op': lambda array, mask: array, |
There was a problem hiding this comment.
Unfortunately, using a lambda like this will break when n_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.
…433-sdr-large-swaths-of-nodata Conflicts: src/natcap/invest/sdr/sdr.py
src/natcap/invest/sdr/sdr.py
Outdated
| func=pygeoprocessing.raster_map, | ||
| kwargs={ | ||
| 'op': lambda array, mask: array, |
There was a problem hiding this comment.
@phargogh should this be _mask_op now in kwargs?
|
I just want to make sure I understand what "mutually masking" means. Are you overlaying all of the raster inputs, and anywhere any of the input layers have NoData, set the rest of the input layers to NoData, including the DEM? And this would be done before any USLE/SDR calculations are begun? This seems fine to me, and we do already say that anywhere there's NoData in the inputs there will be NoData in the outputs. It will be interesting to see what kind of effect doing this has on outputs that rely on upslope/downslope area, which may now be missing data - seems like a lot of NoData propagation might happen. Depending on how this shakes out, I might end up writing more explanation and provide a visual example in the UG. It also brings to the forefront to make sure that we advise on how to fill NoData in all of the input layers that are likely to have them. I think most of this is already done in the UG, but probably not for all relevant inputs. |
@newtpatrol yes, if this change is merged, we are overlaying all of the aligned raster inputs and anywhere any of the aligned layers have a nodata pixel, the whole stack is nodata. Then we use that mask on all of the aligned layers. This guarantees a consistent stack of operable pixels. Yes, this would be done before any calculations are done, including USLE/SDR. |
…433-sdr-large-swaths-of-nodata
This PR addresses two issues that are actually pointing to the same problem, which is that the user is missing data where the DEM is valid. I settled on solving this by masking the user's input to avoid needing to support users who don't read any documentation about this. No change should be needed in the User's Guide as a result of this change.
Fixes #1433
Fixes #911
@emlys and @dcdenu4, both of you were involved in the discussion in #911 , so I wanted to be sure you saw this.
@newtpatrol, feel free to review if you would like! I mostly wanted to include you here as an FYI.
Checklist
- [ ] Tested the Workbench UI (if relevant)