New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grid based interface fixes #794

Merged
merged 26 commits into from Jul 27, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Jul 18, 2016

Ensure that the flat and non-flat arrays returned by the grid interfaces are consistent and of the correct orientation. Also ensures the constructors work correctly and all process tuples of the coordinates and arrays correctly. Also adds small fixes for the GridImage Element, updates test and fixes issues with dynamic groupby, which caused the grid interface to reject the data in some cases. Addresses #789 and more.

Still want to add a number of tests to ensure consistency and stability of the interfaces.

  • Standardized interface to get canonical views of coordinates and data arrays
  • Fixed constructors ensuring they handle standard tuple and dict based gridded data correctly
  • Fixed GridImage density calculation
  • Fixes for dynamic groupby with gridded interfaces (and allowing passing of parameters to groups).
  • Added unit tests to check output of gridded data dimension_values is consistent.
np.flipud(np.array([[ 0, 4, 8],
[ 1, 5, 9],
[ 2, 6, 10],
[ 3, 7, 11]], dtype=np.int32).T))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

As long as you are sure this is definitely correct now... :-)

@@ -183,7 +183,7 @@ def update_handles(self, key, axis, element, ranges, style):
class ImagePlot(RasterPlot):
def get_data(self, element, ranges, style):
data = element.dimension_values(2, flat=False)
data = np.flipud(element.dimension_values(2, flat=False))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

How come this doesn't flip Image given dimension_values for numpy arrays won't be based on interface classes?

Edit: I now realize this isn't RasterPlot and is only handling GridImage so ignore this comment!

xsampling = (float(r-l)/xs)/2.
ysampling = (float(t-b)/ys)/2.
xsampling = (float(r-l)/(xs-1))/2.
ysampling = (float(t-b)/(ys-1))/2.

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

There was an off-by-one error in the bounds? How come this wasn't flagged up by the tests...or did you update the test data? If not, why isn't bounds being checked in the data comparisons?

This comment has been minimized.

@philippjfr

philippjfr Jul 26, 2016

Member

This was on GridImage, and is currently untested.

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

Ah ok, that makes sense.

vdims = param.List(default=['z'], bounds=(1, 1))
vdims = param.List(default=[Dimension('z')], bounds=(1, 1))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

I thought strings were automatically promoted to Dimension objects and that we favored strings by default?

This comment has been minimized.

@philippjfr

philippjfr Jul 26, 2016

Member

Not as a default.

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

Looking at various examples in element I see you are right. Now I'm wondering how it worked at all before when using the default!

@@ -396,6 +397,8 @@ def __init__(self, data, extents=None, **params):
def _compute_raster(self):
if issubclass(self.interface, GridInterface):
return self, np.flipud(self.dimension_values(2, flat=False))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

Perhaps all interfaces could have a property to check to see if this return can be done directly instead of using isinstance?

def load_subset(*args):
constraint = dict(zip(dim_names, args))
group = self.select(**constraint)
if np.isscalar(group):
return group_type(([group],), group=self.group,
label=self.label, vdims=self.vdims)
return group_type(group.reindex(group_dims))
return group_type(group.reindex(group_dims), **group_kwargs)
dynamic_dims = [d(values=list(self.interface.values(self, d.name, False)))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

I think this looks right, but if I am not confused this is a separate fix from the rest of the PR? Looks like the kwargs weren't being passed into the group container when dynamic....

This comment has been minimized.

@philippjfr

philippjfr Jul 26, 2016

Member

That caused weird bugs on the interfaces before I fixed the grid interfaces. So it's related at least.

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

I'm happy to have the fix! I just wanted to make sure I understood it...

@@ -56,7 +56,8 @@ def init(cls, eltype, data, kdims, vdims):
data = {k: data[:,i] for i,k in enumerate(dimensions)}
elif isinstance(data, list) and np.isscalar(data[0]):
data = {dimensions[0]: np.arange(len(data)), dimensions[1]: data}
elif not isinstance(data, dict):
elif not any(isinstance(data, tuple(t for t in interface.types if t is not None))

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

I would just add a comment to say this is needed as dictionary can incorrectly accept xarray (duck typing fails here!). The comment might also mention that a cleaner approach would be welcome...

@classmethod
def expanded_coords(cls, dataset, dim):

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

Can't this be folded into get_coords above using an expanded=False default argument?

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

Also, I've never been a fan of get_ as a prefix. How about simple coords or coordinates?

This comment has been minimized.

@philippjfr

philippjfr Jul 26, 2016

Member

Sure, just coords is fine.

"""
Canonicalize takes an array of values as input and
reorients and transposes it to match the canonical
format expected by plotting functions. In addition

This comment has been minimized.

@jlstevens

jlstevens Jul 26, 2016

Member

It is probably too much of a pain to stick in the docstring but it would be good to describe our canonical format properly somewhere in our docs. Not something that should hold up this PR though!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 26, 2016

Looks good!

I've made most of my key comments now and in general I am happy with this. I didn't look in detail at the changes in the interface methods that already exist - I assume those are fixes as opposed to API changes.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 26, 2016

Thanks for the review.

I assume those are fixes as opposed to API changes.

Yes mostly changes to correctly handle constant dimensions, actually integrating constant dimensions properly might actually be helpful here in future.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 26, 2016

actually integrating constant dimensions properly might actually be helpful here in future.

Does xarray, iris etc have a corresponding concept? As I think they might, in a separate PR, we might want to consider a minimal API for setting/accessing constant dimensions on interfaces.

It is a little tricky in some cases e.g wrapping numpy arrays + bounds as in such a case, I'm not sure where this state would actually live.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 26, 2016

Does xarray, iris etc have a corresponding concept? As I think they might, in a separate PR, we might want to consider a minimal API for setting/accessing constant dimensions on interfaces.

Iris does have explicit support and xarray at least highlights which dimensions are constant in the repr. The special handling is mostly about removing the constant axes in the array, e.g. reducing an (100, 50, 1, 1) shaped array to (100, 50). But it would be nice not to throw those constant coordinates away and turn them into constant dimensions instead.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 27, 2016

I have now applied all your comments except for folding in expanded_coords. As it is now it can be implemented once on the GridInterface baseclass, if I fold it into the coords method I'll be duplicating it for each of the coords implementations.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

if I fold it into the coords method I'll be duplicating it for each of the coords implementations.

Would that be true if you made expanded_coords into a utility?

I can imagine each coords implementation having the signature:

def coords(cls, dataset, dim, ordered=False, expanded=False):

and then all ending with something like:

return util.expanded_coords(data, expanded)

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 27, 2016

Okay done that as well now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

Looks good! I'll merge as soon as the tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

Tests passed. Merging.

@jlstevens jlstevens merged commit 843eeed into master Jul 27, 2016

3 of 4 checks passed

coverage/coveralls Coverage decreased (-22.8%) to 46.626%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Jul 27, 2016

@philippjfr philippjfr added this to the v1.6.1 milestone Jul 27, 2016

@philippjfr philippjfr deleted the interface_fixes branch Sep 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment