Skip to content

Preserve deferred loading of netCDF aux coords.#1291

Closed
rhattersley wants to merge 3 commits into
SciTools:v1.7.xfrom
rhattersley:lazy-dtype
Closed

Preserve deferred loading of netCDF aux coords.#1291
rhattersley wants to merge 3 commits into
SciTools:v1.7.xfrom
rhattersley:lazy-dtype

Conversation

@rhattersley

Copy link
Copy Markdown
Member

Fix for #1287.

(Replaces #1290.)

@rhattersley

Copy link
Copy Markdown
Member Author

@ocefpaf

the problem is just delayed. Right?

Yes. 😒

It fixes the issue with load_cube being slower than load_raw, but the deferred loading doesn't survive the indexing operation. Looks like I'll have to go the whole hog and use biggus instead...

@ocefpaf

ocefpaf commented Aug 27, 2014

Copy link
Copy Markdown
Member

Here is the test:

http://nbviewer.ipython.org/gist/ocefpaf/e8f60b7eb040b1aee0e7

Is there a way to slice without creating the auxiliary coordinate? A more elegant solution would be for aux_factory.py to create the coordinate just for the sliced data. (Sorry I cannot be more helpful, but some parts of iris code base are way beyond my limited python knowledge.)

@rhattersley

Copy link
Copy Markdown
Member Author

Is there a way to slice without creating the auxiliary coordinate?

Biggus 😁

@rhattersley

Copy link
Copy Markdown
Member Author

Hopefully #1293 will get travis working.

@rhattersley

Copy link
Copy Markdown
Member Author

@ocefpaf (and @rsignell-usgs) ... the latest commit should get us pretty close.

@rhattersley rhattersley changed the title Add dtype to LazyArray. Preserve deferred loading of netCDF aux coords. Aug 27, 2014
@ocefpaf

ocefpaf commented Aug 27, 2014

Copy link
Copy Markdown
Member

Yes, things are back to normal (with the exception of the travis failures above 😄). Thanks a lot!

http://nbviewer.ipython.org/gist/ocefpaf/e8f60b7eb040b1aee0e7

PS: Just read what @pelson wrote on biggus to try to understand what is happing in the code. Amazing stuff!

@rhattersley

Copy link
Copy Markdown
Member Author

I've rebased to incorporate #1293 with the aim of getting the tests to pass.

@rhattersley

Copy link
Copy Markdown
Member Author

Travis says 👍 😄

@esc24

esc24 commented Aug 28, 2014

Copy link
Copy Markdown
Member

Super. I'm on it.

Comment thread lib/iris/aux_factory.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be good to add the dtype to the repr.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On reflection - we shouldn't be changing the repr in a bug fix release. Similarly, we shouldn't be changing the API of LazyArray. I'd be more comfortable putting this onto master. If you really want in 1.7.x, then we could make the dtype a keyword arg, but that doesn't make a lot of sense to me.

@pelson

pelson commented Aug 28, 2014

Copy link
Copy Markdown
Member

Whilst I agree that these changes are excellent, they are not in the spirit of a patch release (v1.7.2). That is not to say that this is not something we should do (I desperately want to see this problem resolved) - it is just that the concept of having a LTS point release is a little broken IMHO...

Comment thread lib/iris/aux_factory.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure about this. How about instantiating a new one and setting _array manually?

@esc24

esc24 commented Aug 28, 2014

Copy link
Copy Markdown
Member

I can sympathise with @pelson's view that this is a big change to put into 1.7.x. I can justify it to myself in terms as a bug fix to the LTS release as I see it fixing a recently added, unintended side-effect. However, the change to a public API concerns me.

@rsignell-usgs

Copy link
Copy Markdown

Don't rush on my account I have conda module for Wakari and I'm happy for
now

On Thursday, August 28, 2014, Ed Campbell notifications@github.com wrote:

I can sympathise with @pelson https://github.com/pelson's view that
this is a big change to put into 1.7.x. I can justify it to myself in terms
as a bug fix to the LTS release as I see it fixing a recently added,
unintended side-effect. However, the change to a public API concerns me.


Reply to this email directly or view it on GitHub
#1291 (comment).

Dr. Richard P. Signell (508) 457-2229
USGS, 384 Woods Hole Rd.
Woods Hole, MA 02543-1598

@rhattersley

Copy link
Copy Markdown
Member Author

this is a big change to put into 1.7.x

Agreed - it's too big. From a semantic version point of view, it doesn't fit the criteria for a patch release.

I've removed some unnecessary code (related to pickle/copy) which helps, but there is still the issue of the new dtype argument to LazyArray. Possibly the ability of an AuxCoord to accept a biggus Array is also a problem.

I'm less and less convinced that the goal of deferred slicing is compatible with a patch release.

@rhattersley

Copy link
Copy Markdown
Member Author

I'm less and less convinced that the goal of deferred slicing is compatible with a patch release.

@rsignell-usgs - would it cause you a problem if this was targetted at v1.8 instead?

@rsignell-usgs

Copy link
Copy Markdown

@rhattersley No problem for v1.8. I can use the conda module built from this pull request until then.

@pelson

pelson commented Sep 18, 2014

Copy link
Copy Markdown
Member

would it cause you a problem if this was targetted at v1.8 instead?

Would it cause a problem to release a v1.8 early? 😉

@rsignell-usgs

Copy link
Copy Markdown

👍

@rhattersley rhattersley added this to the v1.8 milestone Oct 17, 2014
@ocefpaf

ocefpaf commented Dec 17, 2014

Copy link
Copy Markdown
Member

@rhattersley I tried to using the latest iris checkout and notice that things are still slow. Did this get merged somewhere else? Or is this the only PR currently implementing this?

@rhattersley

Copy link
Copy Markdown
Member Author

is this the only PR currently implementing this?

Yes, sorry. 😒 It needs re-targeting back at master. It slipped off my urgent to-do list because @rsignell-usgs had his own conda build.

@ocefpaf

ocefpaf commented Dec 17, 2014

Copy link
Copy Markdown
Member

I know (I built it 😉) but things are getting a little awkward as iris evolves and we are still relying on an old branch just for that feature.

@rhattersley

Copy link
Copy Markdown
Member Author

things are getting a little awkward

Right then - I'll have to see if I can make progress on it then! (Unless you're about to volunteer ... 😉)

@ocefpaf

ocefpaf commented Dec 18, 2014

Copy link
Copy Markdown
Member

I can try to copy what you did here on a fresh master checkout and create a new PR. But I am afraid that my "git mergetool" is not so good to rebase this PR . (I know because I tried 😬 and got lost with the changes.) Is that OK for you? If so I will start right away.

@rhattersley

Copy link
Copy Markdown
Member Author

Is that OK for you?

Yes - that's fine. Feel free to do whatever is easiest!

@ocefpaf

ocefpaf commented Dec 18, 2014

Copy link
Copy Markdown
Member

Working on it...

@ocefpaf ocefpaf mentioned this pull request Dec 18, 2014
@ocefpaf

ocefpaf commented Dec 18, 2014

Copy link
Copy Markdown
Member

Done #1502.

@rhattersley

Copy link
Copy Markdown
Member Author

Now handled by #1502.

@rhattersley rhattersley deleted the lazy-dtype branch April 28, 2016 13:05
@QuLogic QuLogic removed this from the v1.8 milestone Sep 8, 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.

6 participants