Open
Conversation
Member
sadielbartholomew
left a comment
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, |
Member
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):
Suggested change
| cell_overlaps, | |
| celloverlaps, |
|
|
||
| def ne(value, units=None, attr=None, exact=True): | ||
| """A `Query` object for a "not equal" condition. | ||
| """A condition for "not equalto a value". |
Member
There was a problem hiding this comment.
Suggested change
| """A condition for "not equalto a value". | |
| """A condition for "not equal to a value". |
| def cell_overlaps(value0, value1, units=None): | ||
| """A condition for "part of cell overlaps with a range". | ||
|
|
||
| The range is closed, meaning that is inclusive of its end points. |
Member
There was a problem hiding this comment.
Suggested change
| The range is closed, meaning that is inclusive of its end points. | |
| The range is closed, meaning that it is inclusive of its end | |
| points. |
| def cellwi(value0, value1, units=None): | ||
| """A condition for "cell lies completely within a range". | ||
|
|
||
| The range is closed, meaning that is inclusive of its end points. |
Member
There was a problem hiding this comment.
Suggested change
| The range is closed, meaning that is inclusive of its end points. | |
| The range is closed, meaning that it is inclusive of its end | |
| points. |
| """A `Query` object for a "cell bounds lie without range" condition. | ||
| """A condition for "cell lies completely outside a range". | ||
|
|
||
| The range is closed, meaning that is inclusive of its end points. |
Member
There was a problem hiding this comment.
Suggested change
| The range is closed, meaning that is inclusive of its end points. | |
| The range is closed, meaning that it is inclusive of its end | |
| points. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #824