Skip to content

Revert "Use atleast_3d instead of ensure_3d" #5241

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

Closed
wants to merge 1 commit into from

Conversation

cimarronm
Copy link
Contributor

This reverts commit 2f5a633.
numpy.atleast_3d does not convert an empty array properly.

This reverts commit 2f5a633.
numpy.atleast_3d does not convert an empty array properly.
@cimarronm
Copy link
Contributor Author

proposed fix for issue #5185

@mdboom
Copy link
Member

mdboom commented Oct 14, 2015

Cc: @jkseppan, as the author of the original commit.

@tacaswell tacaswell added this to the next point release (1.5.0) milestone Oct 14, 2015
@tacaswell
Copy link
Member

The doc failure is fixed on 1.5.x, will merge over now.

I am still unclear on why this results in the intermittent failure and I could not reproduce this in linux.

@jkseppan
Copy link
Member

I'm fine with this going in, but it sounds odd that this would fix an intermittent failure. Is the use of atleast_3d causing reads of uninitialized data in our extension code? If so, I would suspect that the extension code is still broken in some way.

@jenshnielsen
Copy link
Member

@jkseppan Reading uninitialized data sounds like the only explanation I can think of. Unfortunately I don't have any experience with running valgrind or such on python extensions

@mdboom
Copy link
Member

mdboom commented Oct 14, 2015

I guess our understanding of this comes down to:

EDIT: Sorry, missed your explanation in #5185. Still puzzling, though.

@cimarronm
Copy link
Contributor Author

I believe it is due to out-of-bound memory access which is causing this random behavior. I tried to give some more insight in #5185 on what is happening in the path c++ extension code where it goes bad.

@mdboom
Copy link
Member

mdboom commented Oct 20, 2015

Superceded by #5274. @cimarronm: Thanks for all your help identifying the issue!

@mdboom mdboom closed this Oct 20, 2015
mdboom added a commit to mdboom/matplotlib that referenced this pull request Oct 22, 2015
mathtext creates Python dictionaries for the charmap and inverse charmap
for each font.  This turns out to be unnecessary:

1) freetype has an API to do a charmap lookup that is faster than a
Python dictionary

2) The inverse charmap isn't really necessary if we convert the
latex_to_bakoma to use unicode character points rather than glyph
indices.

This should have a large impact when matplotlib#5241 is merged with larger fonts.
mdboom added a commit to mdboom/matplotlib that referenced this pull request Oct 26, 2015
mathtext creates Python dictionaries for the charmap and inverse charmap
for each font.  This turns out to be unnecessary:

1) freetype has an API to do a charmap lookup that is faster than a
Python dictionary

2) The inverse charmap isn't really necessary if we convert the
latex_to_bakoma to use unicode character points rather than glyph
indices.

This should have a large impact when matplotlib#5241 is merged with larger fonts.
mdboom added a commit to mdboom/matplotlib that referenced this pull request Oct 27, 2015
mathtext creates Python dictionaries for the charmap and inverse charmap
for each font.  This turns out to be unnecessary:

1) freetype has an API to do a charmap lookup that is faster than a
Python dictionary

2) The inverse charmap isn't really necessary if we convert the
latex_to_bakoma to use unicode character points rather than glyph
indices.

This should have a large impact when matplotlib#5241 is merged with larger fonts.
mdboom added a commit to mdboom/matplotlib that referenced this pull request Oct 27, 2015
mathtext creates Python dictionaries for the charmap and inverse charmap
for each font.  This turns out to be unnecessary:

1) freetype has an API to do a charmap lookup that is faster than a
Python dictionary

2) The inverse charmap isn't really necessary if we convert the
latex_to_bakoma to use unicode character points rather than glyph
indices.

This should have a large impact when matplotlib#5241 is merged with larger fonts.
mdboom added a commit to mdboom/matplotlib that referenced this pull request Oct 28, 2015
mathtext creates Python dictionaries for the charmap and inverse charmap
for each font.  This turns out to be unnecessary:

1) freetype has an API to do a charmap lookup that is faster than a
Python dictionary

2) The inverse charmap isn't really necessary if we convert the
latex_to_bakoma to use unicode character points rather than glyph
indices.

This should have a large impact when matplotlib#5241 is merged with larger fonts.
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.

5 participants