Skip to content

Added tolerances to Coord.is_contiguous()#459

Merged
pelson merged 1 commit intoSciTools:masterfrom
esc24:is_contiguous_tol
Apr 18, 2013
Merged

Added tolerances to Coord.is_contiguous()#459
pelson merged 1 commit intoSciTools:masterfrom
esc24:is_contiguous_tol

Conversation

@esc24
Copy link
Member

@esc24 esc24 commented Apr 16, 2013

The iris.coords.Coord.is_contiguous() method tests for equality between adjacent upper and lower bounds to determine whether bounds are contiguous. This runs into issues with floating point numbers. For example:

import numpy as np
import iris

delta = np.float64(0.00001)
lower = -1.0 + delta
upper = 3.0 - delta
points, step = np.linspace(lower, upper, 2,
                           endpoint=False, retstep=True)
points += step * 0.5 
coord = iris.coords.DimCoord(points)
coord.guess_bounds()
coord.is_contiguous()

returns False.

This PR modifies is_contiguous() to use np.allclose() adding the tolerance keyword args to is_contiguous().

@ghost ghost assigned bblay Apr 17, 2013
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to reproduce numpy's defaults here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot come up with a neat way not to. Feel free to suggest one, but I'm happy to have clearly stated, documented defaults and simple code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bblay - do you have any outstanding objections to this being merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies @esc24 but I could not finish the review.
My comment about defaults, above, was just wondering if we could just pass through None instead,
and refer to the np func for defaults. This would allow np to change them and we'd stay up-to-date.

@pelson
Copy link
Member

pelson commented Apr 17, 2013

👍 - I'd be happy to merge this.

@esc24 esc24 mentioned this pull request Apr 18, 2013
pelson added a commit that referenced this pull request Apr 18, 2013
Added tolerances to Coord.is_contiguous()
@pelson pelson merged commit 3fc1f72 into SciTools:master Apr 18, 2013
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a comment in here as this is an important test, perhaps something like,
bounds[0,1] differs from bounds[1,0] around the 16th decimal place.
Alternatively, it might make the test clearer if you were to simply hard-code the values?

@bblay
Copy link
Contributor

bblay commented Apr 18, 2013

👍 There are a few trivial comments you might choose to act on, but I'd be happy for this to be merged now.
I'd be happier still with a response to the 'defaults' question:
"can't we just pass None through to np, so it can decide (and change) appropriate defaults?".
Perhaps we can't, I'm sure I've seen functions that can't be used like that.
Perhaps we don't want to rely on np incase it breaks our tests in the future.

@pelson
Copy link
Member

pelson commented Apr 18, 2013

but I'd be happy for this to be merged now.

I'm guessing you missed the fact that it had already been merged 😄. Its up to you if you'd like Ed to follow up on those comments in a follow-up PR.

I'd be happier still with a response to the 'defaults' question

No, you can't do this. None is a legitimate argument and almost all functions (unless None means "use a default value" and is typically only done for mutable default values).

@bblay
Copy link
Contributor

bblay commented Apr 19, 2013

Doh! 😊 They removed the red box saying merged.
Those comments are not important enough to require more work on this.
Thanks Phil

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.

3 participants