Use of tuples for dimensions in coord to dim mapping #124
Conversation
|
Looks good. +1 once there is an entry in the changelog. :-) |
|
It'd be nice to have a quick test of the defensive, immutable properties. e.g. Pass in a list when constructing a Cube, modify the list, check the Cube is still OK. |
ghost
assigned
Do we even have that in general? I can't imagine that the cube's data is a copy from the one I might have given it - therefore I assume I could directly set it's |
|
I'm going to close this, write a short test, add something to the changelog/what's new and re-open. |
Firstly, we're just as fallible as anyone else. There's a chance we'll write code which relies on bad behaviour. So we should try to prevent that where possible. Secondly, once enough people start using something it doesn't matter if it's undocumented, private, silly, whatever ... changing it will cause them pain, and it's our job to reduce pain levels, not increase them. ;-) |
…n lists to prevent inadvertent side effects from mutable type. Cube.coord_dims() now returns a tuple of ints.
There was a problem hiding this comment.
It would have been fine to simply do some type checking here (i.e. assert it is a tuple).
There was a problem hiding this comment.
Arguably the current check is better - it's testing the desired behaviour, and could conceivably survive a switch to an alternative immutable type.
Use of tuples for dimensions in coord to dim mapping
There was a problem hiding this comment.
This stuff is already handled in the load API PR.
There was a problem hiding this comment.
Sorry already merged. Would you mind re-basing the load api PR and handling this conflict (and I will then merge).
3 participants
Modifications to cube.py so that the coord to dimensions mappings (
_aux_coords_and_dimsand_dim_coords_and_dims) now use tuples rather than lists. This should prevent inadvertent side effects. Cube.coord_dims() now returns a tuple of ints.cube.xml_element()explicitly converts to a list for xml output (there was a mix of tuples and lists before).