Skip to content

PI-3483: Make changes to coord_comparison private #3558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stephenworsley wants to merge 3 commits into from
Closed

PI-3483: Make changes to coord_comparison private #3558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63cda3d
make changes to coord_comparison private
stephenworsley Nov 21, 2019
b0b9889
Added docstring
stephenworsley Nov 21, 2019
e0ba0dd
add whitespace
stephenworsley Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 63 additions & 50 deletions lib/iris/analysis/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,58 +186,14 @@ def matches_any(self, predicate):
return any(self.matches(predicate))


def coord_comparison(*cubes, object_get=None):
def _dimensional_metadata_comparison(*cubes, object_get=None):
Copy link
Member

@pp-mo pp-mo Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !!

Copy link
Member

@bjlittle bjlittle Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@stephenworsley stephenworsley Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@stephenworsley stephenworsley Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@pp-mo pp-mo Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😈

"""
Convenience function to help compare coordinates on one or more cubes
by their metadata.

Return a dictionary where the key represents the statement,
"Given these cubes list the coordinates which,
when grouped by metadata, are/have..."

Keys:

* grouped_coords
A list of coordinate groups of all the coordinates grouped together
by their coordinate definition
* ungroupable
A list of coordinate groups which contain at least one None,
meaning not all Cubes provide an equivalent coordinate
* not_equal
A list of coordinate groups of which not all are equal
(superset of ungroupable)
* no_data_dimension
A list of coordinate groups of which all have no data dimensions on
their respective cubes
* scalar
A list of coordinate groups of which all have shape (1, )
* non_equal_data_dimension
A list of coordinate groups of which not all have the same
data dimension on their respective cubes
* non_equal_shape
A list of coordinate groups of which not all have the same shape
* equal_data_dimension
A list of coordinate groups of which all have the same data dimension
on their respective cubes
* equal
A list of coordinate groups of which all are equal
* ungroupable_and_dimensioned
A list of coordinate groups of which not all cubes had an equivalent
(in metadata) coordinate which also describe a data dimension
* dimensioned
A list of coordinate groups of which all describe a data dimension on
their respective cubes
* ignorable
A list of scalar, ungroupable non_equal coordinate groups
* resamplable
A list of equal, different data dimensioned coordinate groups
* transposable
A list of non equal, same data dimensioned, non scalar coordinate groups
This is a generalised form of :func:`coord_comparison`. See that function
for a more detailed description.

Example usage::

result = coord_comparison(cube1, cube2)
print('All equal coordinates: ', result['equal'])
Additionally, this function can compare over different types of dimensional
metadata with the argument object_get. For example, :func:`coord_comparison`
effectively has object_get=:meth:`iris.cube.Cube.coords`.

"""
if object_get is None:
Expand Down Expand Up @@ -397,6 +353,63 @@ def no_data_dim_fn(cube, coord):
return result


def coord_comparison(*cubes):
"""
Convenience function to help compare coordinates on one or more cubes
by their metadata.

Return a dictionary where the key represents the statement,
"Given these cubes list the coordinates which,
when grouped by metadata, are/have..."

Keys:

* grouped_coords
A list of coordinate groups of all the coordinates grouped together
by their coordinate definition
* ungroupable
A list of coordinate groups which contain at least one None,
meaning not all Cubes provide an equivalent coordinate
* not_equal
A list of coordinate groups of which not all are equal
(superset of ungroupable)
* no_data_dimension
A list of coordinate groups of which all have no data dimensions on
their respective cubes
* scalar
A list of coordinate groups of which all have shape (1, )
* non_equal_data_dimension
A list of coordinate groups of which not all have the same
data dimension on their respective cubes
* non_equal_shape
A list of coordinate groups of which not all have the same shape
* equal_data_dimension
A list of coordinate groups of which all have the same data dimension
on their respective cubes
* equal
A list of coordinate groups of which all are equal
* ungroupable_and_dimensioned
A list of coordinate groups of which not all cubes had an equivalent
(in metadata) coordinate which also describe a data dimension
* dimensioned
A list of coordinate groups of which all describe a data dimension on
their respective cubes
* ignorable
A list of scalar, ungroupable non_equal coordinate groups
* resamplable
A list of equal, different data dimensioned coordinate groups
* transposable
A list of non equal, same data dimensioned, non scalar coordinate groups

Example usage::

result = coord_comparison(cube1, cube2)
print('All equal coordinates: ', result['equal'])

"""
return _dimensional_metadata_comparison(*cubes)


class _Aggregator:
"""
The :class:`_Aggregator` base class provides common aggregation
Expand Down
12 changes: 6 additions & 6 deletions lib/iris/cube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3575,23 +3575,23 @@ def __eq__(self, other):
)

if result:
coord_comparison = iris.analysis.coord_comparison(
comparison = iris.analysis._dimensional_metadata_comparison(
Copy link
Member

@bjlittle bjlittle Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@pp-mo pp-mo Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self, other, object_get=Cube.cell_measures,
)
# if there are any cell measures which are not equal
result = not (
coord_comparison["not_equal"]
or coord_comparison["non_equal_data_dimension"]
comparison["not_equal"]
or comparison["non_equal_data_dimension"]
)

if result:
coord_comparison = iris.analysis.coord_comparison(
comparison = iris.analysis._dimensional_metadata_comparison(
self, other, object_get=Cube.ancillary_variables,
)
# if there are any ancillary variables which are not equal
result = not (
coord_comparison["not_equal"]
or coord_comparison["non_equal_data_dimension"]
comparison["not_equal"]
or comparison["non_equal_data_dimension"]
)

# Having checked everything else, check approximate data equality.
Expand Down