Skip to content

Lazy dtype update#1502

Closed
ocefpaf wants to merge 6 commits into
SciTools:masterfrom
ocefpaf:lazy-dtype_update
Closed

Lazy dtype update#1502
ocefpaf wants to merge 6 commits into
SciTools:masterfrom
ocefpaf:lazy-dtype_update

Conversation

@ocefpaf

@ocefpaf ocefpaf commented Dec 18, 2014

Copy link
Copy Markdown
Member

Fix for #1287, replaces #1290 and #1291.

Demo notebook:
http://nbviewer.ipython.org/gist/ocefpaf/4c81ca2d4a86c69e2026

@rhattersley Not sure if I merged things correctly. A quick test with my dataset works 😁 though.

@rhattersley

Copy link
Copy Markdown
Member

Looks like you've rescued all the right changes from my old PR. Re-reading all the old comments it never really got finalised, so there are still a couple of issues to sort out. For example, updating the LazyArray repr. And ideally the new argument to LazyArray would be optional so it remains backwards compatible - I'm not sure how feasible that is though.

@ocefpaf

ocefpaf commented Dec 18, 2014

Copy link
Copy Markdown
Member Author

Looks like you've rescued all the right changes from my old PR.

Thanks to stackoverflow and some googling 😜

For example, updating the LazyArray repr.

Something like this?

    def __repr__(self):
        return '<LazyArray(shape={}, dtype={})>'.format(self.shape, self.dtype)

And ideally the new argument to LazyArray would be optional so it remains backwards compatible - I'm not sure how feasible that is though.

I do not understand iris enough to know if this is possible (probably not, right?):

def __init__(self, shape, func, dtype=None):

How bad it is to change the API for LazyArray? Is it used in other places besides lib/iris/aux_factory.py?

@rhattersley

Copy link
Copy Markdown
Member

How bad it is to change the API for LazyArray? Is it used in other places besides lib/iris/aux_factory.py?

It's not used within Iris, but as it's technically part of the public API it could be used in user code. We're trying to be good citizens and avoid the risk of breaking code where possible. I've sent you ocefpaf#1 which makes it optional (and tweaks 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.

Note to self: this is odd! Why switch? There's already a file for testing build_dimension_coordinate.

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.

Looks like some strangeness here, and while we're making notes, it would be nice to fix the typo: auxilliary -> auxiliary.

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.

This change is coming from 604c1f3, but if you look at e2d8569 (the corresponding commit from #1291) this change is not there! Looks like something went wrong with the extraction from #1291.

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.

I suspect the problem crept in because e2d8569 has a merge conflict with the current master.

@ocefpaf

ocefpaf commented Jan 20, 2015

Copy link
Copy Markdown
Member Author

Note to self: this is odd! Why switch? There's already a file for testing build_dimension_coordinate.

@rhattersley Is this just a matter of renaming it back to build_auxilliary_coordinate? Is there something that I can do here to help?

@ocefpaf

ocefpaf commented Jan 21, 2015

Copy link
Copy Markdown
Member Author

Question:

In lib/iris/fileformats/_pyke_rules/fc_rules_cf.krb there are build_auxiliary_coordinate (that this PR changed to use biggus) and build_dimension_coordinate (that remains unchanged).

Should both make use of biggus? I tried copying the same logic and rebase against the latest master (that has the new vertical ocean coordinates) but failed with MemoryError when loading a ocean_sigma dataset:

Notebook: http://nbviewer.ipython.org/gist/ocefpaf/4fe047bdaeb4a627d4a0

PR: #1533

@rhattersley

Copy link
Copy Markdown
Member

Should both make use of biggus?

For now I'd avoid the extra complexity of using biggus for dimension coordinates. If it becomes a problem we can reconsider. But dimension coordinates are limited to 1-dimension, so they don't grow as a large as the auxiliary coordinates.

@rsignell-usgs

Copy link
Copy Markdown

@rhattersley, pleading for help here. We can't use the official SciTools Iris for many of our workflows until this is merged -- it just blows memory. Here's an example of a notebook that won't run for you, but does run for us with our "custom" Iris version:
http://nbviewer.ipython.org/gist/rsignell-usgs/7eea48ac4124a7071473

@rhattersley

Copy link
Copy Markdown
Member

@rhattersley, pleading for help here.

I've attempted to pull everything together and fix the weird test issue mentioned above. It's now in #1560. I think that gives us a decent candidate for merging with all the known issues addressed.

@ocefpaf ocefpaf closed this Mar 12, 2015
@ocefpaf ocefpaf deleted the lazy-dtype_update branch March 12, 2015 14:58
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.

4 participants