Skip to content

Conversation

stephenworsley
Copy link
Contributor

This pull request currently shows an approach to solving #3529 which, while failed, I think is still instructive.

I tried comparing _ancillary_variables_and_dims and _cell_measures_and_dims in a way which would ignore their order as lists. This first attempt involved simply turning these lists into sets. This failed because the objects inside the lists are not viable elements of a set.

One idea I am coming around to, if such a comparison is deemed necessary, is to use the code in this function:

def coord_comparison(*cubes):
perhaps creating a generic version which compares generic aspects of a cube, then having coord_comparison and something like ancil_comparison wrap this generic function. I'm leaning towards this idea since it seems as though coord_comparison is designed to be more efficient than a more naive, brute force approach. If we needed to compare many ancillary variables (which seems like a thing we would probably want to do) it seems likely we would end up writing something similar to what already exists in this code anyway.

@stephenworsley
Copy link
Contributor Author

After having a bit more of a look at coord_comparison, this may not be the most efficient way to determine equality either, since dimensions within the cube are checked for equality after the data in the coords are checked for equality. There's still a case to make a generic function that checks equality of various *_and dims properties.

@pp-mo
Copy link
Member

pp-mo commented Nov 14, 2019

This first attempt involved simply turning these lists into sets. This failed because the objects inside the lists are not viable elements of a set.

I think you've mis-diagnosed what was wrong here.

The errors with set creation occur because the elements of cube._ancillary_variables_and_dims are of the form [ancil-var-object, dims-tuple].
I think that is just a mistake. The entries in that list should be 2-tuples (var, dims), not lists.
So the problem is just this line, which I reckon should be making a 2-tuple instead of a 2-element list.

And the same thing needs fixing for cell measures here.
( I suspect this is yet another case where the cell-measures implementation was slightly wrong, we never noticed, we copied it, and now we are having trouble ! ... )

If you fix those, I think the set approach will work.
It seems to be mostly working when I try it (modulo a few more unrelated problems?)
See: #3537

I really like this approach (if you can make it work), because it provides all the needed logic based only+entirely on equality testing, which being a standard approach is very easy to understand.

@pp-mo pp-mo mentioned this pull request Nov 14, 2019
@pp-mo
Copy link
Member

pp-mo commented Nov 15, 2019

WARNING
@bjlittle / @rhattersley pointed out that the ordering of cell-methods is significant.
#3530
So in that case we can have simple list equality.

@stephenworsley
Copy link
Contributor Author

@pp-mo For consistency, I have made the same change to dim_coords_and_dims and aux_coords_and_dims. Also, I have added a test to make sure reordering cell methods causes equality to fail.

@stephenworsley stephenworsley changed the title WIP: Fix equality (DO NOT MERGE) Fix equality Nov 18, 2019
cube1.add_cell_method(cmth2)
cube2.add_cell_method(cmth2)
cube2.add_cell_method(cmth1)
self.assertFalse(cube1 == cube2)
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a successful comparison with cell-methods here, or is that covered somewhere else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is capturing the behaviour mentioned in the comments on #3530. Adding cell_methods in different orders should cause equality to fail.

Copy link
Member

@pp-mo pp-mo Nov 19, 2019

Choose a reason for hiding this comment

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

Misunderstanding I think !
What I meant to say was : do we need another testcase showing that equality testing succeeds, when the cubes have cell methods, but they do match.
( Unless that is clearly covered somewhere else ?? )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yeah, that sounds appropriate.

@trexfeathers trexfeathers self-assigned this Nov 19, 2019
@pp-mo
Copy link
Member

pp-mo commented Nov 19, 2019

Looking very close, I think I'd still push for a test of successful cell-method comparisons, OWOK ! 😁

@pp-mo pp-mo merged commit 83a939d into SciTools:master Nov 19, 2019
@pp-mo
Copy link
Member

pp-mo commented Nov 19, 2019

Good stuff 👍

@stephenworsley
Copy link
Contributor Author

Closes #3529.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants