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

Iris interface handles standard constructors enabling unit tests #709

Merged
merged 4 commits into from Jun 9, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Jun 4, 2016

This PR allows the iris interface to handle the standard tuple and dict based constructors making it equivalent to the grid interface. This means all the unit tests, except for aggregation and sampling (which are not yet implemented), can be shared across the two interfaces.

@philippjfr philippjfr changed the title from Iris handles standard constructors enabling unit tests to Iris interface handles standard constructors enabling unit tests Jun 4, 2016

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 6, 2016

Fixes various inconsistencies and adds better coverage. Ready to merge.

self.data_instance_type = iris.cube.Cube
self.init_data()
def test_dataset_add_dimensions_values_hm(self):

This comment has been minimized.

@jlstevens

jlstevens Jun 8, 2016

Member

I assume you are doing this to disable some of the inherited tests. A docstring explaining why these tests are not enable would be good...

This comment has been minimized.

@philippjfr

philippjfr Jun 8, 2016

Member

Yes, sounds good. They should eventually be remove once those methods are implemented.

@@ -70,7 +70,7 @@ def test_select_index(self):
def test_select_slice(self):
cube = Dataset(self.cube)
self.assertEqual(cube.select(longitude=(0, 1)).data.data,
self.assertEqual(cube.select(longitude=(0, 1.01)).data.data,

This comment has been minimized.

@jlstevens

jlstevens Jun 8, 2016

Member

Is the extra .01 due to inclusive vs exclusive bounds?

This comment has been minimized.

@jlstevens

jlstevens Jun 8, 2016

Member

If so, it might be worth defining self.epsilon and using that.

@@ -232,7 +253,7 @@ def select_to_constraint(cls, selection):
if isinstance(constraint, slice):
constraint = (constraint.start, constraint.stop)
if isinstance(constraint, tuple):
constraint = iris.util.between(*constraint)
constraint = iris.util.between(*constraint, rh_inclusive=False)

This comment has been minimized.

@jlstevens

jlstevens Jun 8, 2016

Member

Ah, this must be related to the bounds issue I noticed in the tests (see below).

This comment has been minimized.

@philippjfr

philippjfr Jun 8, 2016

Member

Yes, so far we've used non-inclusive upper bounds in holoviews, so this just makes it consistent. Might be worth having the discussion which is more appropriate, since iris and xarray both seem to use inclusive bounds.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 8, 2016

Looks good and good to support those constructors. My only worry is won't changing the way it does inclusive/exclusive bounds be an issue for backwards compatibility? Perhaps I'm missing something...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 8, 2016

My only worry is won't changing the way it does inclusive/exclusive bounds be an issue for backwards compatibility?

Backwards compatibility for the Iris backend? I guess so, but it was a bug since it was inconsistent with the other interfaces.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 8, 2016

Ok, as long as that is clear. I agree consistency is more important, especially as this interface is new.

I'm happy to merge now, although maybe you might like to implement my self.epsilon suggestion? Defining something called epsilon helps make it clear that all you want is a small increment to include something in the bounds that would otherwise be excluded.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 8, 2016

I'll do that quickly in addition to adding a comments about the disabled tests.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 9, 2016

Done. Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 9, 2016

Looks good. Merging.

@jlstevens jlstevens merged commit 16d97f4 into master Jun 9, 2016

4 checks passed

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

@philippjfr philippjfr removed the in progress label Jun 9, 2016

@jlstevens jlstevens deleted the iris_constructor branch Jul 12, 2016

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