cf.aggregate "cells" keyword
#674
Conversation
…hon into equals-performance-2
There was a problem hiding this comment.
All good, in general. Implements the requested feature in an elegant way. The testing and documentation for the new functionality is really thorough which is great.
I've raised a selection of minor comments and a question or two as well. But once you've considered those, feel free to merge. As a few general points to add to the minor comments in-line, I've noticed in the CI jobs that:
- some
_isclosetests are failing on both Ubuntu jobs which finished here, which could imply that the logic isn't consistent across environments:
======================================================================
FAIL: test_Query_isclose (test_Query.QueryTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/cf-python/cf-python/main/cf/test/test_Query.py", line 690, in test_Query_isclose
self.assertNotEqual(9.000001, q)
AssertionError: 9.000001 == <CF Query: (isclose 9)>
======================================================================
FAIL: test_Data_isclose (test_Data.DataTest)
Test the `isclose` Data method.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/cf-python/cf-python/main/cf/test/test_Data.py", line 4096, in test_Data_isclose
self.assertFalse(d.isclose(1.1))
AssertionError: <CF Data(): True> is not false
so is probably something that should be looked into. Locally those both always passed for me when I ran the corresponding modules, FYI.
- the linting job is failing so some formatting fixes should ideally be made pre-merge.
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
I vaguely recall that something similar happened in some other tests, and was due to differently represented rounding errors. I suspect that if I change the test to have fewer significant decimal places all will be well. |
Oh, hang on, I see that the |
Probably not, haha 🤔 But since you are off on leave and it would be good to get this merged, perhaps we can put down investigating it as a separate, follow-on issue? Then I can look into it whilst you are away. How does that sound? |
Sounds great :) |
sadielbartholomew
left a comment
There was a problem hiding this comment.
Feedback all addressed well, thanks. I've done a final sanity check on the state of the branch now and all is good. There are a few 'optional' tags to remove in line with the new tweak made to __query_isclose__, as raised in-line, but other than that, we are good to merge.
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
|
Thanks, for the review, Sadie - merging! |
Fixes #452