Skip to content

Make iris.util.new_axis handle lazy data#2051

Merged
bjlittle merged 1 commit intoSciTools:masterfrom
ajdawson:lazy-new-axis
Jun 15, 2016
Merged

Make iris.util.new_axis handle lazy data#2051
bjlittle merged 1 commit intoSciTools:masterfrom
ajdawson:lazy-new-axis

Conversation

@ajdawson
Copy link
Member

Make iris.util.new_axis work without loading a deferred data payload. I've used iris.cube.Cube.lazy_data() for this, I didn't use a check to see if the cube actually has lazy data, it seems not to matter, but advice is welcome.

@ajdawson ajdawson force-pushed the lazy-new-axis branch 2 times, most recently from 2937f5e to 93afdfd Compare June 13, 2016 13:11
@ajdawson
Copy link
Member Author

I'd really like to squeeze this into the next release if possible @bjlittle? (I know you said it is closed for further PRs, but the branch has yet to be cut...).

@bjlittle
Copy link
Member

bjlittle commented Jun 15, 2016

@ajdawson Our motivation was to push 1.10 out asap, but we're hitting issues with iris-grib baggage.

I suspect that this PR could slip in 😉 , but if not we want a swift point release after 1.10. I'll have a huddle with the other devs to discuss. Because we're just not slick enough with the release mechanism "at the moment" it's inevitable that more PRs will come knocking on the door ...

In the meantime, something has broken the channel dependencies. I re-ran your travis tests to check that this was the case ... and sure enough it is.

I can't recreate the problem locally, in a fresh conda env and manually stepping through the .travis.yml just works.

Any ideas? Have you seen this before?

@ajdawson
Copy link
Member Author

This perhaps? conda/conda#2675

@bjlittle
Copy link
Member

Shameful if true ... awesome spot, I'll give it a try, thanks!

@bjlittle
Copy link
Member

bjlittle commented Jun 15, 2016

@ajdawson You never guess what ... that was it 😱

See #2053

If you merge it, and rebase this PR, I'll oblige ... it only seems fair! 😉

@bjlittle bjlittle self-assigned this Jun 15, 2016
@bjlittle
Copy link
Member

bjlittle commented Jun 15, 2016

@ajdawson I think that it would be worth while adding an integration test that actually has a cube with deferred data e.g. iris.load_cube(iris.sample_data_path('air_temp.pp')) and uses iris.util.new_axis to promote a scalar coordinate e.g. pressure, to just make sure that all is well with the world in terms of lazy payload.

What do you think?

Otherwise, I'm 👍

@bjlittle bjlittle added this to the v1.10 milestone Jun 15, 2016
@ajdawson
Copy link
Member Author

I think the integration test is a nice idea. I will do that.

@bjlittle
Copy link
Member

Awesome.

@ajdawson
Copy link
Member Author

I've added an integration test using a file from the test data repository (not from the sample data repo as you suggested @bjlittle 😉). I'll rebase onto #2053 once it is merged (I will assume @dkillick is taking care of this).

cube = iris.load_cube(filename)
new_cube = new_axis(cube, 'time')
self.assertTrue(cube.has_lazy_data())
self.assertTrue(new_cube.has_lazy_data())
Copy link
Member

@bjlittle bjlittle Jun 15, 2016

Choose a reason for hiding this comment

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

@ajdawson Did you also want to check the resulting shape ?

If so, then I guess that would also apply to test_lazy_data ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure why not.

@ajdawson ajdawson force-pushed the lazy-new-axis branch 2 times, most recently from 071b214 to 23965d9 Compare June 15, 2016 14:29
@ajdawson
Copy link
Member Author

OK I've modified the unit and integration tests. They are now essentially the same as each other, but one uses real data.

@bjlittle
Copy link
Member

@ajdawson Looks great to me. Thanks!

Happy for you to squash (now if you want) and rebase (once #2053 is in) 👍

@bjlittle
Copy link
Member

@ajdawson #2053 is in ... over to you!

@bjlittle bjlittle merged commit 056bcf3 into SciTools:master Jun 15, 2016
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.

2 participants