Skip to content

Conversation

@sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Mar 27, 2024

Close #740, where we have come to realise that it might not be a good idea to support the feature for wo queries since there is the potential for a lot of confusion, namely I started to document and implement the following behaviour:

open_lower: bool, optional
If True, open the interval at the lower
bound so that value0 is excluded from the
range and therefore included in the set of
values outside of the range captured by "wo".
By default the interval is closed so that
value0 is included in the range and not
captured by "wo".

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.

@sadielbartholomew sadielbartholomew marked this pull request as ready for review April 9, 2024 13:09
@sadielbartholomew
Copy link
Member Author

@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!

@sadielbartholomew
Copy link
Member Author

Will add a Changelog entry, also. Commit to follow.

@sadielbartholomew
Copy link
Member Author

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).

Copy link
Collaborator

@davidhassell davidhassell left a 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>
@sadielbartholomew
Copy link
Member Author

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.

@sadielbartholomew
Copy link
Member Author

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.

Copy link
Collaborator

@davidhassell davidhassell left a 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.

sadielbartholomew and others added 5 commits April 24, 2024 10:45
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>
@sadielbartholomew sadielbartholomew merged commit 17432f7 into NCAS-CMS:main Apr 24, 2024
@sadielbartholomew sadielbartholomew deleted the open-intervals-wi-wo-query branch April 24, 2024 10:09
@davidhassell davidhassell added this to the 3.16.2 milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support (half)-open intervals for wi query endpoints

2 participants