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

Coding standards tweaks to daskified cube code #2416

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

DPeterK
Copy link
Member

@DPeterK DPeterK commented Mar 6, 2017

There were a few inconsistencies in the cube + dask code, and instead of noting them down for further discussion, I thought I'd make a vessel for discussion (that is, this PR).

A retargetting of #2415 to the base dask_timed.

@DPeterK
Copy link
Member Author

DPeterK commented Mar 6, 2017

See the comments on #2415 for a discussion of why I've made these changes.

@QuLogic QuLogic added this to the dask milestone Mar 6, 2017
@DPeterK
Copy link
Member Author

DPeterK commented Mar 7, 2017

Ping @bjlittle @pp-mo @lbdreyer

@pp-mo
Copy link
Member

pp-mo commented Mar 7, 2017

See the comments on #2415 for a discussion of why I've made these changes.

See related query, here : https://github.com/SciTools/iris/pull/2415/files#r104770825

cube_data = self._dask_array
else:
raise ValueError('This cube has no data, slicing is not supported')
cube_data = self._numpy_array
Copy link
Member

Choose a reason for hiding this comment

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

@dkillick At this point, we're confident that either self._dask_array or self._numpy_array is populated.

Prior to this change, there seems to be a hint of that might not be the case, hence the ValueError exception being raised. I'd certainly hope that there is a data payload of some kind to process - we need to make sure that internally that is always the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if we end up at this point having a cube with no data we're deeper in the sticky than a ValueError will help with! The rest of the code in the class makes it clear that we can never construct a cube with no data, so if an end user has done something that means their cube no longer has any data then IMO they can dig their own way out of the sticky stuff...

Copy link
Member

Choose a reason for hiding this comment

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

I'm really hoping that there never, ever is any sticky stuff 😉

It's just a note to ourselves really, that we need to ensure the integrity of the cube as best we can

@bjlittle
Copy link
Member

bjlittle commented Mar 8, 2017

@dkillick @pp-mo @lbdreyer I'm 👍 on this PR ... are there any reasons why we can't bank this now?

@DPeterK
Copy link
Member Author

DPeterK commented Mar 8, 2017

@bjlittle let's just make sure @pp-mo is happy with my comment at his link above 😄

@pp-mo pp-mo closed this Mar 8, 2017
@DPeterK DPeterK changed the base branch from dask_timed to dask March 8, 2017 12:02
@DPeterK DPeterK reopened this Mar 8, 2017
@DPeterK
Copy link
Member Author

DPeterK commented Mar 8, 2017

@bjlittle I think @pp-mo and I might be happy with this now - we invite you to hit the merge button once Travis is happy!

@DPeterK DPeterK added the dask label Mar 8, 2017
@pp-mo pp-mo merged commit a334540 into SciTools:dask Mar 8, 2017
@DPeterK DPeterK deleted the dask_cube_inconsistencies branch March 8, 2017 14:12
bjlittle pushed a commit to bjlittle/iris that referenced this pull request May 31, 2017
@QuLogic QuLogic modified the milestones: dask, v2.0 Aug 2, 2017
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