Uh oh!
There was an error while loading. Please reload this page.
New function: cf.cell_overlaps - #825
Conversation
sadielbartholomew
left a comment
There was a problem hiding this comment.
Good idea for new methods, should be useful. Implementation and new testing and docs are otherwise all good but I have questioned the inconsistent underscore in the name and I have one general suggestion (plus spotted a few typos):
- Since this new
cell_overlaps, as well ascellwiandcellwowhich arewi/woqueries applied to a cell, it would be nice to include the new-ishopen_lower=False, open_upper=Falsekeywords we added recently to the generalwi/woqueries so they can be made open or half-open. - For the case of
cell_overlapsthe open-/closed-ness could do with being clarified a bit since it's a bit less direct/clear as to the role of interval openness. You state as with the other relevant methods 'The range is closed, meaning that is inclusive of its end points.' so I guess that means if the endpoint of the cell 'overlapped' with an endpoint of the range, then in the closed/inclusive default case it would beTrue, e.g. for cell[5, 10]and range[10, 15]it is deemed to overlap (True)? It would be good to add either a brief sentence or a new docstring example to clarify that.
Happy to approve once you'd considered those. Thanks.
| cellgt, | ||
| cellle, | ||
| celllt, | ||
| cell_overlaps, |
There was a problem hiding this comment.
General comment, but the import listing here emphasises it: all of the other queries are named without underscores, where words are merged without delimiter e.g. gt, cellsize. cellgt and cellwi etc. So I advise for consistency we do the same here (obviously the name change here would need propagating as well as this GH suggestion):
| cell_overlaps, | |
| celloverlaps, |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#824