Modernize and simplify iris.analysis._Groupby

[bouweandela]

🚀 Pull Request

Description

Modernize and simplify iris.analysis._Groupby. This started out from the idea discussed in #4956.

Here is an overview of some of the improvements here:

• Replace iris.coords._GroupIterator by the standard library function itertools.groupby
• Fix the iris.analysis._Group.__repr__ method, it seems currently broken because it uses an attribute that the class doesn't have (anymore). I suspect that this code is not covered by tests Consider using a code coverage service #5014
• Add type hints
• Simplify the code by removing the special treatment of groupby_slices if it had a length of 1
• Simplify the code by removing the unused dictionary keys stored in _Groupby._slices_by_key and instead using a list called _Groupby._groupby_indices
• Simplfiy the code of _Groupby.group by returning an empty list if _Groupby._groupby_coords was empty. Previously there was a special if statement to make it return None, but this behaviour was not used anywhere in the code.

---

Consult Iris pull request check list

[codecov]

Codecov Report

Patch coverage: 95.00% and project coverage change: +0.01 🎉

Comparison is base (6b25c3a) 89.28% compared to head (a70aea2) 89.29%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5015      +/-   ##
==========================================
+ Coverage   89.28%   89.29%   +0.01%     
==========================================
  Files          88       88              
  Lines       22269    22228      -41     
  Branches     4870     4865       -5     
==========================================
- Hits        19882    19849      -33     
+ Misses       1641     1634       -7     
+ Partials      746      745       -1     

Impacted Files	Coverage Δ
lib/iris/analysis/__init__.py	92.44% <94.91%> (+0.54%)	⬆️
lib/iris/coords.py	93.27% <100.00%> (+0.07%)	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[trexfeathers]

Great work @bouweandela, and sorry for the delay.

I'm all for anything maintains functionality while doing this:

❗ This deserves a What's New entry.

Responses to bullet points

• Replace iris.coords._GroupIterator by the standard library function itertools.groupby

👍 From debugging through the old and new version of group(), I can see that this works in an equivalent way.

• Fix the iris.analysis._Group.repr method, it seems currently broken because it uses an attribute that the class doesn't have (anymore). I suspect that this code is not covered by tests #5014

👍 A definite improvement! I've left some comments.

• Add type hints

👍 Many thanks for doing this 🙂 I have checked for consistency.

• Simplify the code by removing the special treatment of groupby_slices if it had a length of 1

👍 I assume you're referring to _slice_merge()? I agree with the decision - see the relevant comment.

• Simplify the code by removing the unused dictionary keys stored in _Groupby._slices_by_key and instead using a list called _Groupby._groupby_indices

👍

• Replace the use of OrderedDict by dict, since dicts are ordered since Python 3.7

❓ I think this point is redundant thanks to making the _Groupby._groupby_indices list?

• Simplfiy the code of _Groupby.group by returning an empty list if _Groupby._groupby_coords was empty. Previously there was a special if statement to make it return None, but this behaviour was not used anywhere in the code.

👍 I think the upstream code protects against there being no coordinates, and the downstream code wouldn't be able to handle None if that was returned.

[trexfeathers]

@bouweandela any comment on my comments?

[bouweandela]

Thanks for reviewing @trexfeathers! I ran out of time for today, will continue tomorrow.

[bouweandela]

@trexfeathers This is ready for another round of reviewing. Could you have a look, please?

[trexfeathers]

Great, thanks @bouweandela 🍻