Skip to content

Revert "Use atleast_3d instead of ensure_3d"#5241

Closed
cimarronm wants to merge 1 commit into
matplotlib:masterfrom
cimarronm:revert_atleast_3d
Closed

Revert "Use atleast_3d instead of ensure_3d"#5241
cimarronm wants to merge 1 commit into
matplotlib:masterfrom
cimarronm:revert_atleast_3d

Conversation

@cimarronm

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

proposed fix for issue #5185

@mdboom

mdboom commented Oct 14, 2015

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

mdboom commented Oct 14, 2015

Copy link
Copy Markdown
Member

I guess our understanding of this comes down to:

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

@cimarronm

Copy link
Copy Markdown
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

mdboom commented Oct 20, 2015

Copy link
Copy Markdown
Member

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