ENH: Added conservative interpolation approach #18

cpelley · 2017-10-30T17:27:20Z

API that closely aligns with the existing stratify API.

def interpolate(z_target, z_src, fz_src, axis=-1):
    ....

Extrapolation doesn't make sense for a conservative approach so intentionally does not use the same API (separate function).
A technical review has already been undertaken as part of this going into ANTS and so it is expected that the review be primarily focused on the changes necessary to migrate this code to python-stratify.
Doesn't currently support case where the target levels is 1D and the source levels is ND which stratify.interpolate supports I believe (the implementation can easily support it but I chose not to at the first pass as we don't need it right now so couldn't justify the extra time writing tests).
Extends the top-level namespace of stratify:

stratify.interpolate_conservative

Cythonised functions where it matters (stratify._conservative.pyx) and a python module for where it doesn't (stratify._bounded_vinterp). Cython code can be further optimised in future (no bounds checking etc.) but intentionally not done here.

cpelley · 2017-10-31T09:16:10Z

ping @hdyson @jkettleb

@hdyson has agreed to perform the first-pass review for this.

Cheers

stickler-ci · 2017-10-31T09:42:00Z

stratify/__init__.py

                       EXTRAPOLATE_NAN, EXTRAPOLATE_NEAREST,
                       EXTRAPOLATE_LINEAR, PyFuncExtrapolator, 
                       PyFuncInterpolator)
+from ._bounded_vinterp import interpolate as interpolate_conservative


F401 '._bounded_vinterp.interpolate as interpolate_conservative' imported but unused

Ignore this

Tell flake8 to ignore it with a # noqa type comment in the code. Ref: http://flake8.pycqa.org/en/3.2.1/user/ignoring-errors.html#in-line-ignoring-errors

Thanks @pelson

cpelley · 2017-10-31T09:55:52Z

Sorry iterating through issues exposed with travis:

Failure due to mock usage and missing from requirements.txt (removed my usage of mock).
Numpy include dirs (I didn't need this building locally). Have added this now to setup.py
Absolute import for python3 support.
Installing a python3 environment locally to remedy remaining issues around python3 support.

cpelley · 2017-10-31T10:18:55Z

Tests now pass for python3 (I'll run these locally in a py3 environment in future).

Ready for review, really this time.

Cheers

hdyson · 2017-11-06T10:32:15Z

stratify/_conservative.pyx

+    #          |------------|         : Target
+    #    |----------|                 : Delta (src_max - src_min)
+    #          |----|                 : Overlap (between src & tgt)
+    #    weight = overlap / delta


I really like this diagram - it provides the key concept and several definitions very succinctly.

hdyson · 2017-11-06T10:35:42Z

stratify/_bounded_vinterp.py

+    if z_src.ndim != z_target.ndim:
+        msg = ('Expecting source and target levels dimensionality to be '
+               'identical.  {} != {}.')
+        raise ValueError(msg.format(z_src.ndim, z_target.ndim))


Would this (and following assertions) be more transparent as:

('Expecting dimensionality of source levels {} and target levels {} to be ' 'identical.')

It's just moving the value of the "thing" to where the definition of the "thing" is.

hdyson · 2017-11-06T10:43:31Z

stratify/_conservative.pyx

+    cdef np.float64_t delta, weight
+    cdef np.ndarray[np.float64_t, ndim = 2] weights
+    cdef np.ndarray[np.float64_t, ndim = 1] src_cell, tgt_cell
+


It could be cython convention that I'm not familiar with, but I'd expect these to be "ndim=2" without spaces? Similarly for lines 35 and 36.

hdyson · 2017-11-06T10:45:27Z

stratify/__init__.py

                       PyFuncInterpolator)
+from ._bounded_vinterp import (interpolate as  # noqa: F401
+    interpolate_conservative)
+


Can this be simplified by renaming _bounded_vinterp.interpolate as _bounded_vinterp.interpolate_conservative?

hdyson · 2017-11-06T10:47:05Z

stratify/_bounded_vinterp.py

+
+
+def interpolate(z_target, z_src, fz_src, axis=-1):
+    """


I've suggested renaming this in another comment. If you opt to retain the existing name, I'd suggest adding that the interpolation is conservative to this docstring?

hdyson · 2017-11-06T10:51:07Z

Thanks @cpelley, this looks good to me. There's a few suggestions above - but I want to be clear that they are just suggestions for some potential fine tuning around the edges.

cpelley · 2017-11-06T16:25:37Z

Thanks @hdyson . Made all changes except the exception messages. I felt that it was clearer especially in some cases and so opted for consistency.

Example:

""The provided data is not of compatible shape with the provided source bounds. ('-', 3, 4) != (2, 4)"

Here '-' is used to denote the dimensions which are not relevant to that statement. Not so clear if pulled into the sentence itself.

pelson

Looking good. I need to carry on reading from _conservative.pyx.

pelson · 2017-11-13T14:45:23Z

stratify/_bounded_vinterp.py

+    invert_transpose = [data_transpose.index(ind) for ind in
+                        list(range(result.ndim))]
+    result = result.transpose(invert_transpose)
+    return result


Comments:

Looks like this could be a class with behaviour (conservative_interpolation).

Very similar code for the non-bounded case. Would be interesting to pull this all out into common functions.

Agreed.

I saw these two points as being connected. I think a refactor to support caching would entail pulling both existing and conservative capability into classes. I didn't want to pre-define a structure before considering this refactor in that wider context of caching (it was easier to use a proxy to the existing API for now). With regards to common functions, as far as I can tell. the existing stratify usage is rather hardcoded to a points based approach right now.
In the refactor to support caching, I hope to have a similar mindset to what I have done here, in that we pull out the code which needn't be cython code into a python module (inc. enduser API), allowing greater flexibility and functionality to be more readily shared between points-based and bounds-based approaches. Are you happy with this mindset?

Cheers

pelson · 2017-11-13T14:46:08Z

stratify/_conservative.pyx

@@ -0,0 +1,55 @@
+import numpy as np


No docstrings 😱

Thanks Phil, just added (see latest commit)
I have taken a numpy doc style approach in the python module but followed the same style forming as the existing cython module for the new conservative cython module. Let me know what you think.

Cheers

pelson

Happy from a maintainability, plumbing and testing perspective.

I'd be interested in seeing the 0 extrapolation behaviour changed though. Thoughts?

pelson · 2017-12-11T15:53:52Z

stratify/tests/test_bounded_vinterp.py

+                                                       source_bounds,
+                                                       self.data, axis=1)
+        target_data = np.ones((4, 7))
+        target_data[:, 0] = 0


Why 0 and not some other extrapolation scheme?

Feels to me like 'nan' (as you say) is more appropriate here. If no source points are contributing to a target cell then there is no data so it should be 'nan'.

I can't think why I chose 0 in the first place and since @hdyson didn't pick up on this either, @hdyson is there a reason that you can remember why 0 seemed to make sense at the time but doesn't now? :)

Cheers

That's going back a while. From memory (so pinch of salt and all that), I seem to recall we discussed this on the phone, but I don't remember the reasons for and against 0.

That said, this behaviour is consistent with the approach we implemented for the CMIP6 aerosols (and was in turn copied from an earlier IDL routine), so it could simply be for consistency with legacy applications. Hope that helps?

Thanks @hdyson I'll change to 'NaN' then, we can always post-process the returned array if it proves to be necessary to the UM from such ancillaries.

pelson · 2017-12-11T15:59:38Z

stratify/tests/test_bounded_vinterp.py

+
+
+if __name__ == '__main__':
+    unittest.main()


Don't see any non-equally spaced coordinate tests.

pelson · 2017-12-11T16:07:06Z

Also, could you add a check for data values being NaN too, please?

cpelley · 2017-12-12T16:41:46Z

Also, could you add a check for data values being NaN too, please?

Yep, will do. Cheers

…ell. TODO: Update tests

cpelley · 2018-01-17T12:32:27Z

Hey @pelson, thanks for your patience.
Three new commits, addressing your concerns:

TEST: Added non-equally spaced coordinate test.
ENH: Return nan values where no source cells contribute to a target cell (I was returning 0 before).
ENH: Return nan values in tgt cells where a src nan cells contribute (this was neither tested before nor supported).

pelson

Looks good to me.

Thanks @cpelley!

pelson · 2018-01-26T13:01:12Z

stratify/tests/test_bounded_vinterp.py

+                                                       source_bounds, data,
+                                                       axis=1)
+        target_data = np.array(
+            [1/1.5, 1 + ((1/3.)/1), 0.25, 0.25, 0.25, 0.25, 1.])[None]


Wowzers. This messes with my mind 😄 .

Looks good though.

pelson · 2018-01-26T13:02:53Z

Thanks for the great review @hdyson. Really appreciated you helping to shape this PR into this really great addition. 👍

cpelley · 2018-01-26T13:13:58Z

Thanks both, appreciated.

cpelley added the enhancement label Oct 30, 2017

cpelley self-assigned this Oct 30, 2017

cpelley requested a review from pelson October 30, 2017 17:27

cpelley force-pushed the ENH_CONSERVATIVE_APPROACH branch from 2aabdec to 47684e3 Compare October 31, 2017 09:41

stickler-ci reviewed Oct 31, 2017

View reviewed changes

SciTools deleted a comment from stickler-ci Oct 31, 2017

ENH: Added conservative interpolation approach

08239e2

cpelley force-pushed the ENH_CONSERVATIVE_APPROACH branch from 47684e3 to 08239e2 Compare October 31, 2017 09:51

ENH: Python3 support

009b38e

TEST: Flake8 F401 error ignore

c87fb50

hdyson reviewed Nov 6, 2017

View reviewed changes

Carwyn Pelley added 2 commits November 6, 2017 16:06

MAINT: Review changes

a5e9e7c

TEST: Extended regexp raised message checks

b0a2c5f

pelson reviewed Nov 13, 2017

View reviewed changes

DOC: Added docstrings

44edf42

pelson approved these changes Dec 11, 2017

View reviewed changes

TEST: Added non-equally spaced coordinate test

20eb3f5

cpelley added 2 commits January 16, 2018 18:27

ENH: Return nan values where no source cells contribute to a target c…

54bb77a

…ell. TODO: Update tests

ENH: Return nan values in tgt cells where a src nan cells contribute

58098b8

MAINT: Slight simplification

bab0fdd

pelson approved these changes Jan 26, 2018

View reviewed changes

pelson merged commit 44135af into SciTools:master Jan 26, 2018

cpelley deleted the ENH_CONSERVATIVE_APPROACH branch January 26, 2018 13:13

ENH: Added conservative interpolation approach #18

ENH: Added conservative interpolation approach #18

Uh oh!

Conversation

cpelley commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpelley commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpelley commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpelley commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdyson commented Nov 6, 2017

Uh oh!

cpelley commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpelley Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpelley Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Dec 11, 2017

Uh oh!

cpelley commented Dec 12, 2017

Uh oh!

cpelley commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Jan 26, 2018

Uh oh!

cpelley commented Oct 30, 2017 •

edited

Loading

cpelley commented Oct 31, 2017 •

edited

Loading

cpelley commented Oct 31, 2017 •

edited

Loading

cpelley commented Nov 6, 2017 •

edited

Loading

cpelley Nov 13, 2017 •

edited

Loading

cpelley Dec 12, 2017 •

edited

Loading

cpelley commented Jan 17, 2018 •

edited

Loading