Back out #3095 and remove the Column __getitem__ override#3929
Closed
taldcroft wants to merge 1 commit intoastropy:masterfrom
Closed
Back out #3095 and remove the Column __getitem__ override#3929taldcroft wants to merge 1 commit intoastropy:masterfrom
taldcroft wants to merge 1 commit intoastropy:masterfrom
Conversation
Member
Author
|
@mdmueller - this is the fix I was talking about today. |
Member
|
How does this actually affect the API? Does it mean it returns single-element |
Member
|
Nevermind, I went back and re-reviewed the discussion around this and looked at the tests, and I remember now. I have a fix for this that I think will retain the performance for the (more common) single-dimensional case, while still getting the desired behavior for the N-d case. Lemme see if I can get that going... |
Member
Author
|
That would be great @embray ! |
embray
added a commit
to embray/astropy
that referenced
this pull request
Jul 9, 2015
…bclasses on an as-needed basis when using those columns to store multi-dimensional data. The new classes have the slower __getitem__ implementation, while the stock classes go straight through ndarray.__getitem__ and retain its performance.
embray
added a commit
to embray/astropy
that referenced
this pull request
Jul 9, 2015
…bclasses on an as-needed basis when using those columns to store multi-dimensional data. The new classes have the slower __getitem__ implementation, while the stock classes go straight through ndarray.__getitem__ and retain its performance.
Member
|
Superseded now by #3930 |
dhomeier
pushed a commit
to dhomeier/astropy
that referenced
this pull request
Aug 11, 2015
…bclasses on an as-needed basis when using those columns to store multi-dimensional data. The new classes have the slower __getitem__ implementation, while the stock classes go straight through ndarray.__getitem__ and retain its performance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #3095 fixed a corner case issue where a single row item access of a multi-dimensional Column returned a Column object instead of the expected ndarray.
However, by adding a pure-Python override of the
ndarray.__getitem__method a significant performance penalty (factor of 10) was incurred for single-element item access (#3232). In the discussion of 3232 it was agreed to back out #3095, but in all the activity related to the 1.0 release this didn't get done.This PR does the back out.