Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Apr 21, 2016

Deprecation of cube.regridded and the whole of iris.analysis.interpolate module.
We basically want all of these older methods updated to the new scheme-based "cube.regrid" and "cube.interpolate".

It's a bit noisy, partly unavoidable due to renames.
It should be clearer from the individual commits.

# Set this as the init for the wrapper class.
initfn.func_name = '__init__'
# Also copy the original docstring.
initfn.__doc__ = parent_class.__init__.__doc__
Copy link
Member

Choose a reason for hiding this comment

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

Probably cleaner to do this with functools.wraps

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 did wonder about that, but I couldn't quite work out how to make it go.
I'll take another look.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps...

@wraps(parent_class.__init__)
def initfn(self, *args, **kwargs):
    print(...)

Copy link
Member Author

@pp-mo pp-mo Apr 21, 2016

Choose a reason for hiding this comment

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

... actually, I've got a problem here...
The documentation is not getting the constructor signatures right for any of my classes wrapped with this 'ClassDeprecationWrapper' : They all appear to be class(*args, **kwargs).
This is not helpful + not what I intended.
I see that the docs of the existing master already has this problem, where I had deprecated iris.fileformats.ff.
I think it relates to : http://stackoverflow.com/a/17705456
Does anyone know how to fix this properly without having to define an independent matching __init__ for each wrapper class ?

Copy link
Member

Choose a reason for hiding this comment

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

how to fix this properly

Use Python 3.4 😉 It's more complicated for Python 2.

@rhattersley
Copy link
Member

Looks like excellent progress - thanks @pp-mo 😄

Good idea to keep using the old code internally so we don't have to transition our internal usage (with the likely impact on test results) in the same PR. 👍

@pp-mo
Copy link
Member Author

pp-mo commented Apr 21, 2016

Thanks @rhattersley
I hope this addresses the outstanding.
I have given up on automatically generating a wrapped __init__ function for wrapper classes, as Sphinx seems to take its constructor signature from any overriding constructor function -- either __init__ or __new__, so I couldn't find any way of making it automatically document the signature of the wrapped __init__ method.
The result is ugly, but it's only "temporary" code.

@pp-mo
Copy link
Member Author

pp-mo commented Apr 21, 2016

I do hope I've fixed it now...

P.S.

documentation is not getting the constructor signatures right

Just to be clear, the point at issue here is not any oddness of Python, or metaclasses, but an oddness of Sphinx. I can fix "help(x)", but not the Sphinx output, apparently because it defines an overall "class constructor signature" in a slightly surprising way.
I think you can probably only really work out what it does by trying it.
Sadly, _that_ isn't odd at all but almost standard practice in that area ...

@rhattersley
Copy link
Member

Just to be clear, the point at issue here is not any oddness of Python, or metaclasses, but an oddness of Sphinx.

Thanks for clarifying. 👍 Given the deprecated classes are transient code and you've already boiled it down to a simple, repeatable pattern I'm OK with leaving your solution as it stands. There's no need to burn extra time on a "fancy" solution that works with every permutation of Python 2/3 and Sphinx.

@rhattersley
Copy link
Member

@pp-mo - I'm 👍 for the content. With hindsight, can we easily squash this down to a small number (but more than one) of coherent commits? If not, I think I'd rather merge it as it stands than squash down to a single commit.

@rhattersley rhattersley added this to the v1.10 milestone Apr 25, 2016
@rhattersley
Copy link
Member

can we easily squash this down to a small number (but more than one) of coherent commits?

Nudge...?

@pp-mo
Copy link
Member Author

pp-mo commented Apr 28, 2016

Nudge...?

Thanks @rhattersley. Coming...

@pp-mo pp-mo force-pushed the deprecate_interpolate branch from 1a7fa2f to bd0e11e Compare April 28, 2016 16:30
@pp-mo
Copy link
Member Author

pp-mo commented Apr 28, 2016

can we easily squash this down to a small number (but more than one) of coherent commits?

Now done that, or something like it.
But shortly, no, it was a total beast to do !!

@rhattersley rhattersley merged commit ab07de6 into SciTools:master Apr 29, 2016
@rhattersley
Copy link
Member

Thanks @pp-mo 👍 😄

@pp-mo pp-mo deleted the deprecate_interpolate branch May 11, 2016 14:10
@pelson
Copy link
Member

pelson commented Dec 15, 2016

Just following this up now (I know, how many months ago), but I'm not sure it is clear what functionality has replaced nearest_neighbour_indices. I guess the problem with this "scheme" is that it isn't really an interpolation at all - it is a "find the 'nearest' point (Euclidean distance) and tell me what index I should use to get it". It is actually an extremely useful function that I'd be sad to see gone. @pp-mo - as I say, sorry to drag this back up, but could you please advise?

Thanks!

@pp-mo
Copy link
Member Author

pp-mo commented Dec 16, 2016

what functionality has replaced nearest_neighbour_indices

There's a redirect in the docstring for the deprecated routine. It says ...

Please replace usage of iris.analysis.interpolate.nearest_neighbour_indices() with iris.coords.Coord.nearest_neighbour_index().

But I just realised I messed up the docs build for this in the way I wrapped these routines, so we are no longer building docs for them (!oops)
#2270 addresses

@pp-mo
Copy link
Member Author

pp-mo commented Dec 16, 2016

P.S.

Please replace usage of iris.analysis.interpolate.nearest_neighbour_indices() with iris.coords.Coord.nearest_neighbour_index().

Does this actually address the need you mention, or am I maybe missing something about the usage with multiple sample-point requirements ?

E.G. Here's my equivalent code for the docstring example....

>>> cube = iris.load_cube(iris.sample_data_path('ostia_monthly.nc'))
>>> sample_points = [('latitude', 0), ('longitude', 10)]
>>> indices = nearest_neighbour_indices(cube, sample_points)
>>> indices
(slice(None, None, None), 9, 12)
>>> 
>>> sample_points_dict = dict(sample_points)
>>> indices_2 = tuple([dimcoord.nearest_neighbour_index(sample_points_dict[dimcoord.name()])
...     if dimcoord.name() in sample_points_dict else slice(None)
...     for dimcoord in cube.coords(dim_coords=True)])
>>> indices_2
(slice(None, None, None), 9, 12)
>>> 
>>> indices_2 == indices
True
>>> 

If that's not too horrible ?

@pelson
Copy link
Member

pelson commented Jan 24, 2017

Thanks for your example @pp-mo. Definitely less clear than it was, but I can live with it 😄

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