Skip to content
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.

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

BUG: Tolerant crs comparison #2062

Closed
wants to merge 1 commit into from
Closed

Conversation

cpelley
Copy link

@cpelley cpelley commented Jun 27, 2016

Provide tolerant coordinate system comparison in iris.

Currently, coordinate system comparison in iris has the following issues associated that this PR addresses:

  1. Some attributes for specifying a crs can be specified or derived (this makes floating point comparison flawd) e.g. inverse flattening for defining the ellipse rather than defining both semi major AND minor axes.
  2. crs attributes can be specified as 32-bit floats (consider pp as a source for the instantiation of the crs (see BUG: coord_system datatypes equality beyond original precision #716 for an illustrative case).
  3. crs equality was using the class name in its equality measure.

Solution:

I make a 'tolerant' dict comparison between the coordinate systems (also I remove the requirement for both to be defined from the same class). Tolerance value derived by the unittests written (see lib/iris/tests/unit/coord_systems/test_GeogCS.py).

Context:

In our current project, we are monkey-patching the iris.coord_system.Coor.__eq__ method in order that all coordinate systems can be handled effectively. This PR is about removing the need for our own project hack.

Closes #716

DO NOT MERGE

if True in [dict2[key] is None, value is None]:
result = dict2[key] == value
elif np.isreal(dict2[key]) and np.isreal(value):
result = abs(dict2[key] - value) < 1
Copy link
Author

@cpelley cpelley Jun 27, 2016

Choose a reason for hiding this comment

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

This is an intentionally naive/very accepting here. Feel free to share ideas of what might make floating point comparison here more suitable (i.e. numbers over what ranges and scales are deemed significant as different etc.). I suggest that any solution must address both acceptance criteria.

@cpelley
Copy link
Author

cpelley commented Jul 26, 2016

I have intentionally not changed those existing tests which are effected by this change. It would be good to get agreement in principle that this change seems reasonable first.

I think a tolerance comparison between most objects which contain floats makes better sense to me (especially in the case here where we have derived values). What may likely be more controversial is how to decide on a 'suitable' tolerance value(s).

Cheers

@cpelley
Copy link
Author

cpelley commented Aug 16, 2016

Is there no interest in this change?
I hope we won't have to monkey-patch the iris.coord_system.Coord.__eq__ forever 😞

Having better equality between iris objects is a continuing problem for us as iris generally does not perform equality involving floating point numbers in a practical way (no tolerance).

@cpelley
Copy link
Author

cpelley commented Feb 28, 2017

Closing as I don't see any interest on this.

@cpelley cpelley closed this Feb 28, 2017
@jkettleb
Copy link
Contributor

Is it clear why this isn't a good idea and so not worthy including in iris? I can't see any comments giving us any indication why is not been picked up.

@cpelley
Copy link
Author

cpelley commented Mar 1, 2017

Hi @jkettleb

Thanks for the prod, I should have provided some context:

I began a wider discussion around float handling in iris some time ago:
https://groups.google.com/forum/#!topic/scitools-iris-dev/aD4cessUZ8A

I should probably stress that I my view remains that iris float handling is a serious problem (if not the most problematic we face). After all, we and our users experience many issues as a result, some mild (failures), some serious (silent continuation with incorrect results). This PR was intended to provide a local short term fix for a small subset of the problem in order to allow us to remove a monkey-patch we have for iris in our package.
The discussion on the google forums intended to expose the problem package wide and offer a hypothetical solution (framework) to solving this package wide problem and spark discussion. Unfortunately, from the forum, I don't see that the problem of float handling in iris is understood or indeed it's implications.

I don't see how we can gain traction on this PR while we can't agree that there is a serious problem to fix... :(
I won't delete this branch in case this sparks some interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants