Skip to content

Conversation

@rcomer
Copy link
Member

@rcomer rcomer commented Jun 18, 2019

See #3334. Just removed the first merge to see if anything breaks!

@bjlittle bjlittle self-assigned this Jul 8, 2019
@bjlittle bjlittle added this to the v2.3.0 milestone Jul 9, 2019
@bjlittle
Copy link
Member

bjlittle commented Jul 9, 2019

@rcomer Thanks for this.

Yup, so it appears there is a double merge being performed here within iris.load_cube 😱

Once by the underlying iris.cube._CubeFilter.merged for the cubes associated with the matching constraint. Then once afterwards within iris.load_cube itself, when it calls merge_cube on the iris.cube.CubeList returned from the filter.

This current functionality all works just fine, but is really a tad inefficient. Plus, it results in a not so informative traceback from merge_cube due to the prior (and unnecessary) merge.

With regards to the example given in #3334, we have a traceback that goes from:

iris.exceptions.ConstraintMismatchError: failed to merge into a single cube.
  cube.shape differs: (3, 4) != (2, 4)

to:

ConstraintMismatchError: failed to merge into a single cube.
  Coordinates in cube.aux_coords (non-scalar) differ: baz.

For me, the second traceback wins the day. From a users perspective, they have a fighting chance to understand what the heck is wrong.

LGTM 👍

@bjlittle
Copy link
Member

bjlittle commented Jul 9, 2019

@rcomer I'm more than happy to merge this, but first I need to fix the master travis-ci tests, which are failing at the moment 😢

So, let me do that first, then we can merge this with confidence. Does that sound like a reasonable plan?

@rcomer
Copy link
Member Author

rcomer commented Jul 9, 2019

@bjlittle sounds good to me 👍

@bjlittle
Copy link
Member

bjlittle commented Jul 9, 2019

@rcomer Fancy doing a re-base?

Just to make sure all is well before merging...

Reference #3349

@rcomer rcomer force-pushed the load-cube-single-merge branch from fbec6fe to eebd1d5 Compare July 9, 2019 15:06
@rcomer
Copy link
Member Author

rcomer commented Jul 9, 2019

Rebased 🤞

@bjlittle
Copy link
Member

bjlittle commented Jul 9, 2019

It's been a long day, clumsily clicked the thumbs-down emoji by mistake 😊

@bjlittle
Copy link
Member

bjlittle commented Jul 9, 2019

@rcomer Awesome, thanks 👍

@bjlittle bjlittle merged commit e688664 into SciTools:master Jul 9, 2019
@rcomer rcomer deleted the load-cube-single-merge branch July 10, 2019 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants