Skip to content

Remove invalid dimension checking in axes_rgb. #7826

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

Merged
merged 1 commit into from
Jan 15, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 14, 2017

Just let dstack error out if dimensions don't match.

See #7810.

@codecov-io
Copy link

codecov-io commented Jan 14, 2017

Current coverage is 62.10% (diff: 0.00%)

Merging #7826 into master will increase coverage by <.01%

@@             master      #7826   diff @@
==========================================
  Files           174        174          
  Lines         56052      56049     -3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          34809      34809          
+ Misses        21243      21240     -3   
  Partials          0          0          

Powered by Codecov. Last update 9edcb03...ab9610e

@NelleV
Copy link
Member

NelleV commented Jan 14, 2017

I am concerned that this patch makes it harder for the user to understand where exactly the error comes from.

@tacaswell
Copy link
Member

I agree with @NelleV This is one of the few places where we raise a non-inscrutable exception. This also replaces comparing equality of a few integers with copying all of the data an extra time. It is probably better to just fix the transposition.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Jan 15, 2017
@anntzer
Copy link
Contributor Author

anntzer commented Jan 15, 2017

  1. The error is not really worse than what it effectively was before, given that before, matching nonsquare shapes would be incorrectly rejected and some nonvalid nonsquare shapes would even error a bit further below (although there are certainly cases where the previous case would raise an informative message).

  2. In general I'd rather push for numpy to add these clearer error messages ("cannot stack arrays of shape (12, 34) and (56, 78)"), which would benefit every call site of every project that uses numpy rather than having to rewrite it every time.

  3. np.dstack([r, g, b]) certainly checks sizes before allocating the memory and starting to copy it, so there isn't a performance issue.

@tacaswell
Copy link
Member

  1. fair, but a smaller change would yield a much better warning

  2. also fair, but ValueError: all the input array dimensions except for the concatenation axis must match exactly is what you currently get and given how slow both us and numpy move, it will likely be that way for a while so it is worth our effort to patch around it.

  3. but in the case where everything is copacetic we have an extra set of copies (I think)

Just let `dstack` error out if dimensions don't match.
@anntzer anntzer force-pushed the invalid-rgbaxes-dimensions-check branch from ab9610e to 8e6d4c1 Compare January 15, 2017 05:22
@anntzer
Copy link
Contributor Author

anntzer commented Jan 15, 2017

I feel a bit ridiculous to have waste both of our times over this so I restored the check :) but at least I learnt a new word (copacetic)...
By the way even in that case there is no extra copy, you just trade it against the RGB=R+G+B a bit below... which was likely a bit less efficient.

@tacaswell tacaswell changed the title Remove invalid dimension checking in axes_rgb. [MRG+1] Remove invalid dimension checking in axes_rgb. Jan 15, 2017
@tacaswell
Copy link
Member

ah, I missed the sum at the bottom 🐑

@efiring efiring changed the title [MRG+1] Remove invalid dimension checking in axes_rgb. Remove invalid dimension checking in axes_rgb. Jan 15, 2017
@efiring efiring merged commit b602713 into matplotlib:master Jan 15, 2017
@anntzer anntzer deleted the invalid-rgbaxes-dimensions-check branch January 15, 2017 08:36
@QuLogic
Copy link
Member

QuLogic commented Jan 30, 2017

Backported to v2.0.x as 2da8a98.

QuLogic pushed a commit that referenced this pull request Jan 30, 2017
Remove invalid dimension checking in axes_rgb.
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