-
Notifications
You must be signed in to change notification settings - Fork 23
Open and half-open intervals for wi queries
#753
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
Open and half-open intervals for wi queries
#753
Conversation
|
@davidhassell as suspected after our discussion earlier, there wasn't anything wrong with the compound querying open/closeed-ness - the tests were failing due to a copy-and-paste error on my part: diff --git a/cf/test/test_Query.py b/cf/test/test_Query.py
index 553cbace8..3e891aa20 100644
--- a/cf/test/test_Query.py
+++ b/cf/test/test_Query.py
@@ -501,13 +501,13 @@ class QueryTest(unittest.TestCase):
all_c = [c, c0, c1, c2, c3]
d = cf.wi(6, 8)
- d0 = cf.wi(2, 4, open_lower=False) # equivalent to d, to check default
- d1 = cf.wi(2, 4, open_lower=True)
- d2 = cf.wi(2, 4, open_upper=True)
- d3 = cf.wi(2, 4, open_lower=True, open_upper=True)
+ d0 = cf.wi(6, 8, open_lower=False) # equivalent to d, to check default
+ d1 = cf.wi(6, 8, open_lower=True)
+ d2 = cf.wi(6, 8, open_upper=True)
+ d3 = cf.wi(6, 8, open_lower=True, open_upper=True)🤦♀️ It only took about an hour to work this out 😬 Anyway I have fixed that and all the tests passed. So, now ready for review! |
|
Will add a Changelog entry, also. Commit to follow. |
|
Sorry David I forgot to tag you as requested reviewer last week. All good to review (I resolved one trivial merge conflict in the Changelog for that last merge-based commit). |
davidhassell
left a comment
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.
H Sadie, Thanks! All good logically, apart from a few missing pieces in:
__dask_tokenize__
__str__
equals
It's be good to talk about the API, too.
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
|
Thanks David for your useful feedback and offline discussions on this from last week. Updating my local branch now and I'll push everything up when ready for the re-review. |
|
I have just pushed my new commits which I believe address all of your feedback, thanks @davidhassell. All ready to (rock and) roll with a re-review. |
davidhassell
left a comment
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.
Thanks, Sadie. This looks great, and there's some excellent testing :). I've made some minor style and docstring suggestions. Please merge when when you have considered them.
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
Co-authored-by: David Hassell <david.hassell@ncas.ac.uk>
Close #740, where we have come to realise that it might not be a good idea to support the feature for
woqueries since there is the potential for a lot of confusion, namely I started to document and implement the following behaviour:and @davidhassell and I decided it might be too confusing when applied to "outside a range" as opposed to "inside a range", where it is quite intuitive. So I will update the Issue description accordingly.