PI-3483: Make changes to coord_comparison private #3558
Conversation
stephenworsley
force-pushed
the
eq_tidy
branch
from
be89b34 to
63cda3d
Compare
| print('All equal coordinates: ', result['equal']) | ||
| """ | ||
| def _dimensional_metadata_comparison(*cubes, object_get=None): |
There was a problem hiding this comment.
Just add a comment or docstring to explain
- this is a 'generalised form' of the coord_comparison function
- see there for a detailed behaviour description (especially, what it returns)
- the 'object_get' is a fetch-all function for cube _DimensionalMetadata contents, like
Cube.coords()(which is the default).
Sorry, don't know how to make this really short !!
There was a problem hiding this comment.
@stephenworsley I don't quite understand the reason for making this private?
Wouldn't there be general utility in making this public and exposing this behaviour?
There was a problem hiding this comment.
@bjlittle My thinking behind making it private is that this should always serve as the most generic version of comparison for dimensional metadata. If it needs to change in the future to make it more generic, there is room to do so. To expose the new comparison behaviour, would it make sense to create specific public functions cell_measure_comparison and ancillary_variable_comparison instead?
There was a problem hiding this comment.
@bjlittle With that said, much of the output form _dimensional_metadata_comparison reads as coordinate specific. If this function were to be made public somehow, I think this would need to be addressed.
There was a problem hiding this comment.
Personally, I'm not convinced that we need public functionality for this.
If we do want that, then I don't like either the existing form with the magic 'object_get' keyword, or the prospect of additional specific routines for cell_measure_comparison and ancillary_variable_comparison.
Instead, I would push for a general-purpose dimensional_metadata_comparison routine with a somewhat friendlier API, maybe an element_type='coordinate' (string) keyword.
But in the end, I think we can just not do that, for now.
And that is clearly less work 😈
|
|
||
| if result: | ||
| coord_comparison = iris.analysis.coord_comparison( | ||
| comparison = iris.analysis._dimensional_metadata_comparison( |
There was a problem hiding this comment.
@stephenworsley Seems like we're baking in some indirection here, where the behaviour of our public API is now depending on the private behaviour of _dimensional_metadata_comparison.
This means that indirectly we can change the public API behaviour of iris.cube.Cube.__eq__ by changing iris.analysis.__init__._dimensional_metadata_comparison... that seems a tad dangerous to me.
There was a problem hiding this comment.
we can change the public API behaviour of iris.cube.Cube.eq by changing iris.analysis.init._dimensional_metadata_comparison
I don't see why this is a particular concern. It should be obvious that we wouldn't change the behaviour of the public function without very careful considering + management, and I think the public API dependence on the private function should be clear enough to any potential developer that it wouldn't happen by accident.
Of course, if we make the more general private function public all such problems would go away. But IMHO that seems like a good deal more work that we just don't really need to commit to at this stage.
abooton
changed the title
|
#3562 aims to replace, following the above discussions. Can we close this ? |
|
@pp-mo Sounds fair to me. |
5 participants
This addresses comments on #3546. At a minimum, this causes
coord_comparisonto have the same behaviour as it did previously. The internals of the function still refer to coords rather than, say, ancillary variables. Ideally there would be a more generic name to use as keys for the generic function (preferably one less cumbersome than 'dimensional metadata').