Closed debboutr closed 4 years ago
Totals | |
---|---|
Change from base Build 280: | 0.04% |
Covered Lines: | 2028 |
Relevant Lines: | 2489 |
Makes sense to me. Could you allow for passing in a raster mask as well?
That could come from another from_raster
call and then just be applied to the raster you want to mask.
grid.mask = another_grid.{data name}
then you could run catchment
with apply_mask=True
and you'd be good to go.
Normally with add_gridded_data
there was no mask set when using from_raster
. There are likely potential issues when doing the above, because the user would have to handle the shape and the metadata, but that can be done pretty simply with the window
flag I would think?
We can keep it as a vector mask if passing in an array makes things too complicated. Just let me know when you have something that you're comfortable with merging.
Eventually, I wanted to rewrite the mask object to be a Raster rather than an array. I haven't taken it on yet, though, because of how much of the codebase it would affect.
@mdbartos about making it a Raster object, what advantages would that offer? I just pushed up a test and there are a couple of questions that I have.
raise
an error from rasterio instead? and have it fail? As it is now, it'll just give back the mask that would have been produced if the geometry was never used.nonzero
method and the other just uses counts of True/False in the 2-d array, the second one is simpler so I left that one uncommented, let me know if the other one seems better. Let me know if you can think of any other case that might be a problem. Thanks!
Looks good!
I think returning a warning is probably the right move.
I'd be inclined to use the first test, as it is more specific.
As for making the mask a raster: consider the case where the user changes the view (e.g. through grid.set_bbox) and the bounds of the new view are no longer aligned with the bounds of the current mask. Currently, the mask is simply reset in this case. However, I think it would be preferable to return the portion of the mask that still lies within the new view. If the mask were a Raster, this could be accomplished by calling grid.view(mask). However, this is outside the scope of this PR.
Thanks for that explanation to use Raster, I just went through the startup.ipynb and got the data to run it. I'll have to take a deeper dive into the view
method. I have mostly been using this package to create sub-catchments from NHDPlusV21 catchments based on a pour point for the National Aquatic Resource Survey to use in conjunction with StreamCat data.
It might be good to start an issue to solve it, that way we could have a place to talk about how to approach the problem, cause I imagine what I might think of wouldn't consider many of the cases that are needed. Or it might be good to create assertions in tests of what we want and then try to solve individual tests, I just noticed that the test_set_bbox
makes no assertions, would it help to create some there and in other tests so as we make changes we would see failures? Just a few thoughts...
Thanks for putting this all together, seems like it's helping a lot of people. Let me know if there is anything else I can do with this PR before you merge.
Greetings,
Sorry for the delay---I had to turn in my dissertation last week. I'll review and merge this by the end of the day.
Thanks again for your contributions, MDB
Awesome. Thanks again for the contribution.
As you mentioned, some of the existing tests should probably be improved, and there are certain general changes I'd like to make to the library but haven't had the time.
Feel free to email me if you have ideas for future contributions or want to discuss the future of the project in general. It's great to have skilled developers who want to help.
Hey Matt, I've been playing around with getting the
catchment
method to clip to a vector geometry and had this idea for setting a mask on theRaster
to use in conjunction w/ theapply_mask
attr. I'm not sure if this is the best way to go about doing this, but it has worked with what I'm doing. Let me know if you think this might be a good direction to go in or if there is a better place to try to implement something like this. Seems like we'd have to get the geometry mask figured out at read in so we can handle it with rasterio, otherwise, I think it might be a lot tougher.Thanks!