Removal of a cube's cell measure by name (#3081) #3295
Conversation
alastair-gemmell
force-pushed
the
bugfix-for-issue-3081
branch
from
185ba1d to
dbb02ac
Compare
|
Welcome to Iris-devs !!! I think this is a bug : The docstring says you can pass a name, but you can't. Send us a PR, if you fancy having a go ?? |
|
Tests look ok at first glance, either way. Most important part of any PR. 😉 |
bjlittle
left a comment
bjlittle
left a comment
There was a problem hiding this comment.
@alastair-gemmell Thanks for the PR, and apologies for the tardy response.
Just one minor comment to address, then we'll get this merged! 👍
lib/iris/cube.py
Outdated
| factory.update(coord) | ||
|
|
||
| def remove_cell_measure(self, cell_measure): | ||
| def remove_cell_measure(self, name_or_cell_measure): |
There was a problem hiding this comment.
@alastair-gemmell I think that keeping the cell_measure positional argument "as is" is good enough, and is in keeping with other remove_ methods that allow a mixture of different types of inputs to match on i.e., see iris.cube.Cube.remove_coord for example.
There was a problem hiding this comment.
Thanks @bjlittle . I've made this change now and the tests have passed.
It's good to have this consistency with other remove methods, although there is already inconsistency because the cell_measures and cell_measure methods do have the argument listed as 'name_or_cell_measure' (which is what I based my original change on!)
| (a) a :attr:`standard_name`, :attr:`long_name`, or | ||
| :attr:`var_name`. Defaults to value of `default` | ||
| (which itself defaults to `unknown`) as defined in | ||
| :class:`iris._cube_coord_common.CFVariableMixin`. |
There was a problem hiding this comment.
@alastair-gemmell A lovely piece of clarification, which explains that cell_measure can be an actual CellMeasure instance or an appropriate string name 😄
|
Awesome work, thanks @alastair-gemmell 👍 |
4 participants
Although labelled as a bug I don't think #3081 is a bug strictly speaking. As far as I can tell its behaviour is as expected from the reference docs at https://scitools.org.uk/iris/docs/latest/iris/iris/cube.html#iris.cube.Cube.remove_cell_measure. In as much as the argument to the method is documented as having to be a CellMeasure instance (not a name of a cell measure).
Having said that it could be a bit misleading as other public Cube methods such as Cube.cell_measure take either a CellMeasure object OR the name of a cell measure (https://scitools.org.uk/iris/docs/latest/iris/iris/cube.html#iris.cube.Cube.cell_measure), and currently if you attempt to pass in the name of a cell measure to be removed it isn't removed, silently
So although maybe not a bug in the strict sense it seemed a good candidate for a minor new feature to accept a name of a cell measure when calling the Cube.remove_cell_measure method
Especially as I'm a newbie contributor let me know what you think, and if I've forgotten anything here (I've updated the docstring, added new tests and added a 'whats new' entry but I may have missed something?)