Skip to content

Unusual comparison in coord contiguity check #4473

@jonseddon

Description

@jonseddon

📰 Custom Issue

In iris.coords.Coord._discontiguity_in_bounds() there's an unusual comparison that I'm struggling to understand the logic of. Please have a look at https://github.com/SciTools/iris/blob/main/lib/iris/coords.py#L1686

diffs_along_axis will be an array of bools, with a value of True when the difference between cells is greater than the tolerance.

points_close_enough is then comparing this array of bools against atol + rtol * cell_size, which is an array of floats, and doesn't seem to be a comparison that you would want to do.

I think that the logic's currently returning the correct result because the calculation of points_close_enough effectively inverts each bool in diffs_along_axis. np.all() only returns True (i.e. the coord is contiguous) when all of it's elements are True (so is a logical-and for inputs of bools). If any diffs_along_axis elements were greater than the tolerance then you will therefore get a False returned by np.all().

Although this does work I was wondering if this may not have been the original intention of this bit of code. If this was the intention then it's not immediately clear to the reader how it actually works and could perhaps do with being simplified. This code was introduced by #3144, merged by @pp-mo.

I'm not totally sure of the intentions of calculating points_close_enough currently and so I don't feel able to create a PR to fix this in case I'm missing anything. A simplification could be to go from:

                diffs_along_axis = diffs_between_cells > (
                    atol + rtol * cell_size
                )

                points_close_enough = diffs_along_axis <= (
                    atol + rtol * cell_size
                )
                contiguous_along_axis = np.all(points_close_enough)
                return diffs_along_axis, contiguous_along_axis

to:

                diff_too_big = diffs_between_cells > (
                    atol + rtol * cell_size
                )

                return diffs_along_axis, not diff_too_big

but I'm not sure if this would be chopping out some functionality that points_close_enough was intended to add. If you are happy with my simplification then please let me know and I can raise a PR. An alternative would be to revert to the logic that existed before #3144. Otherwise I'd be grateful if you could suggest an alternative implementation for this bit of a head scratcher of a piece of code.

Metadata

Metadata

Assignees

No one assigned

    Labels

    StaleA stale issue/pull-request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions