Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Dec 20, 2016

A few functional changes / fixes.
A lot of documentation and testing enhancements.

@pp-mo pp-mo changed the title Unstructured regridder usagetidy Unstructured regridder : usage tidying Dec 20, 2016
@pp-mo pp-mo changed the base branch from master to v1.12.x December 21, 2016 10:30
@pp-mo pp-mo force-pushed the unstructured_regridder_usagetidy branch from 63d639f to bb712e4 Compare December 21, 2016 10:34
@pp-mo pp-mo added this to the v1.12.x milestone Dec 21, 2016
Copy link
Member

@marqh marqh left a comment

Choose a reason for hiding this comment

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

broadly supportive, a few comments and queries to look at please

same units in the source and grid cubes.
.. Note::
Currently only supports regridding, not interpolation.
Copy link
Member

Choose a reason for hiding this comment

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

This distinction isn't immediately obvious to me: interpolation is a numerical method which can support a regrid operation.
As this is the docstring for a class called UnstructuredNearest I suggest that this note is simply not required

Copy link
Member Author

@pp-mo pp-mo Dec 21, 2016

Choose a reason for hiding this comment

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

This distinction isn't immediately obvious to me: interpolation is a numerical method which can support a regrid operation.

What the words 'regrid' and 'interpolate' mean in Iris terms is that cubes support both a 'regrid' and an 'interpolate' method, which can both specify a "scheme".

the docstring for a class called UnstructuredNearest

UnstructuredNearest is a scheme, and these can generally be used with either interpolate or regrid
-- except that this one can't. ( Similarly, neither can PointInCell ).

So, I'd like to keep this statement.

This arrangement is now well established in Iris.
The practical difference is that a "regrid" uses a grid cube as its reference, while "interpolate" takes coordinate values.
But I do agree that the need for both is questionable, and the choice between is nowhere very clearly explained !!

Copy link
Member

Choose a reason for hiding this comment

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

ok, I'm content with this

.. Note::
If the points are longitudes/latitudes, these are handled correctly as
points on the sphere, but the values must be in 'degrees'.
Copy link
Member

Choose a reason for hiding this comment

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

is this protected somewhere? if i supply radians, will a meaningful exception be raised?

Copy link
Member Author

@pp-mo pp-mo Dec 21, 2016

Choose a reason for hiding this comment

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

is this protected somewhere? if i supply radians, will a meaningful exception be raised?

No, it is not checked and will just give wrong answers !

I decided to document this problem rather than fix it, as it is an existing low-level routine.
As this is a private routine I thought that was ok.

Arguably, we should maybe fix the existing analysis.trajectory.interpolate, as it calls this and so exposes the same problem.
? what do you think @marqh ?

I have fixed it in the UnstructuredNearest classes, which now convert everything to degrees (and complain if it fails).

* src_cube:
The :class:`~iris.cube.Cube` defining the source grid.
The X and Y coordinates must be mapped over the same dimensions.
The X and Y coordinates can have any shape, but must be mapped over
Copy link
Member

Choose a reason for hiding this comment

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

excellent

def test_nearest(self):
res = self.src.regrid(self.grid, UnstructuredNearest())
self.assertArrayShapeStats(res, (1, 6, 3, 4), 315.888737, 11.000729)
self.assertArrayShapeStats(res, (1, 6, 3, 4), 315.890808, 11.000724)
Copy link
Member

Choose a reason for hiding this comment

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

are these changes expected?
the value differences are small, but non-zero

Copy link
Member Author

@pp-mo pp-mo Dec 21, 2016

Choose a reason for hiding this comment

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

The operation result was previously actually wrong, because the test data has coords with units of 'radians' (!)
The change is small, I think because all the source points (temperatures) actually have quite similar values.
It just goes to show you have to be careful with statistical tests.

@marqh
Copy link
Member

marqh commented Dec 21, 2016

It just goes to show you have to be careful with statistical tests.

indeed

@marqh
Copy link
Member

marqh commented Dec 21, 2016

it seems there's no outstanding issues here, I am go for launch

cheers @pp-mo

@marqh marqh merged commit 128757b into SciTools:v1.12.x Dec 21, 2016
@pp-mo pp-mo deleted the unstructured_regridder_usagetidy branch January 3, 2017 13:36
@lbdreyer lbdreyer mentioned this pull request Jan 9, 2017
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