NCAS-CMS / cf-python

A CF-compliant Earth Science data analysis library
http://ncas-cms.github.io/cf-python
MIT License
119 stars 19 forks source link

Open and half-open intervals for `wi` queries #753

Closed sadielbartholomew closed 4 months ago

sadielbartholomew commented 5 months ago

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 commented 5 months ago

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

:woman_facepalming: It only took about an hour to work this out :grimacing: Anyway I have fixed that and all the tests passed. So, now ready for review!

sadielbartholomew commented 5 months ago

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

sadielbartholomew commented 4 months ago

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

sadielbartholomew commented 4 months ago

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 commented 4 months ago

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.