Coord points and bounds inheritance. #2553
Conversation
lib/iris/coords.py
Outdated
| self._bounds_dm = DataManager(bounds) | ||
| else: | ||
| self._bounds_dm.data = bounds | ||
|
|
||
| # This is necessary for merging, but probably shouldn't be used otherwise. | ||
| # See #962 and #1772. | ||
| def __hash__(self): |
There was a problem hiding this comment.
I know it's out of the current extent of this PR's changes, but could you (/did you) look into whether we still really need this? If we don't we could practically get rid of AuxCoord entirely.
There was a problem hiding this comment.
I looked into this.
The same code is present in Cube.__hash__ (and a couple of others).
In Python3, the hash operation is actually disabled for user classes that define __eq__, which means you must provide __hash__ to allow, for example, collecting the objects into sets, or using them as dictionary keys.
See: #1772
So it seems we do need this, as otherwise we can't collect Coords into sets etc._
The problem:
This violates a general principle that
"objects which compare equal have the same hash value".
See: https://docs.python.org/2.7/reference/datamodel.html#object.__hash__
So, we should really fix these,
but it probably means changing other areas such as merge, and possibly concatenate.
There was a problem hiding this comment.
P.S. I see that the exact same code appears in DimCoord, so it's another thing that can be moved into Coord. I've now done that.
lib/iris/coords.py
Outdated
|
|
||
| if self._points_dm is not None: | ||
| # Fetch the new core array, as the parent can change it. | ||
| points = self._points_dm._real_array |
There was a problem hiding this comment.
I'm not at all a fan of accessing this private DataManager method. Surely there is a better way of doing this?
lib/iris/coords.py
Outdated
|
|
||
| if self._bounds_dm is not None: | ||
| # Fetch the new core array, as the parent can change it. | ||
| bounds = self._bounds_dm._real_array |
There was a problem hiding this comment.
Again, accessing this private method is a bad idea.
lib/iris/coords.py
Outdated
| self._bounds_dm = None | ||
| else: | ||
| def _bounds_setter(self, bounds): | ||
| if bounds is not None: | ||
| # Ensure we have a realised array of new bounds values. | ||
| copy = is_lazy_data(bounds) | ||
| bounds = as_concrete_data(bounds) | ||
| bounds = np.array(bounds, copy=copy, ndmin=2) |
There was a problem hiding this comment.
You no longer need to set ndmin here (it'll be done by _sanitise_points in the parent).
lib/iris/coords.py
Outdated
| @@ -684,6 +766,13 @@ def _as_defn(self): | |||
| self.units, self.attributes, self.coord_system) | |||
| return defn | |||
|
|
|||
| # Must supply __hash__ as Python 3 does not enable it if __eq__ is defined. | |||
| # NOTE: Violates "objects which compare equal must have the same hash". | |||
| # Currently needed, but not really correct and should be fixed. | |||
There was a problem hiding this comment.
I'd add "for merging" to this comment because otherwise it might not be clear why it is needed (that is, what's using it).
There was a problem hiding this comment.
But in fact I tested what happens when you error its usage, and the resulting errors are not about merge after all.
See : #1772 (comment)
? I could say e.g. "needed so we can use Coords as set members and dictionary keys" ?
There was a problem hiding this comment.
New commit coming that I hope makes this clearer...
lib/iris/coords.py
Outdated
| # Make the array read-only. | ||
| self._points_dm.data.flags.writeable = False | ||
| # Make the array read-only. | ||
| points.flags.writeable = False |
There was a problem hiding this comment.
This will update the underlying array (i.e. self._points_dm.core_data()) as well, right?
There was a problem hiding this comment.
Yes, but we made sure we are dealing with our own view, as stated just above (L1661).
lib/iris/cube.py
Outdated
| # This is necessary for merging, but probably shouldn't be used otherwise. | ||
| # Must supply __hash__ as Python 3 does not enable it if __eq__ is defined. | ||
| # NOTE: Violates "objects which compare equal must have the same hash". | ||
| # Currently needed, but not really correct and should be fixed. |
There was a problem hiding this comment.
Again, you could specify where it is needed.
| # "If a class that overrides __eq__() needs to retain the | ||
| # implementation of __hash__() from a parent class, the interpreter | ||
| # must be told this explicitly". | ||
| __hash__ = Coord.__hash__ |
There was a problem hiding this comment.
I could use "super" here, if you think it would be preferable (it avoids saying who your parent is).
This way is more efficient though 😉
* Refactor Coord inheritance. * Review changes. * Review change -- reword comments for clarity. * Fix Python3-specific problem.
This refactors the inheritance organisation of the
pointsandboundsproperty code amongst Coord / AuxCoord / DimCoord.At present,
DimCoordandAuxCoordboth provide their own independent implementations of points+bounds getters+setters, but there is quite a lot of duplication in that code.This refactor removes the code duplication, by ...
AuxCoordintoCoord.AuxCoordthen becomes an almost trivial descendant ofCoord,DimCoordis now a 'specialised' form that inherits + extends those implementations, adding realisation and DimCoord-specific validity testing.I think this structure makes more sense than the existing.
The knock-on effects are extremely minor : just that some error cases produce a slightly different message.
The recently-extended testing in
iris.tests.unit.coordsencourages me that nothing serious has been broken (!)