Skip to content
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
Merged

Grid based interface fixes #794

merged 26 commits into from
Jul 27, 2016

Conversation

philippjfr
Copy link
Member

@philippjfr 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

Okay done that as well now.

@jlstevens
Copy link
Contributor

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

@jlstevens
Copy link
Contributor

Tests passed. Merging.

@jlstevens jlstevens merged commit 843eeed into master Jul 27, 2016
@philippjfr philippjfr added this to the v1.6.1 milestone Jul 27, 2016
@philippjfr philippjfr deleted the interface_fixes branch September 2, 2016 00:57
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants