8000 Allow adding a table column as a list of mixin-type objects (+ row improvement) by taldcroft · Pull Request #9165 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

Allow adding a table column as a list of mixin-type objects (+ row improvement) #9165

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 14, 2019
Merged

Allow adding a table column as a list of mixin-type objects (+ row improvement) #9165

merged 4 commits into from
Sep 14, 2019

Conversation

taldcroft
Copy link
Member
@taldcroft taldcroft commented Aug 24, 2019 •
edited by mhvk
Loading

This PR started with a desire to allow initializing a table with row data that has Quantity values. But along the way I noticed that the existing legacy way of handling rows is slow and unnecessarily complex:

In [1]: rows = [list(range(100))] * 100
In [2]: from astropy.table.np_utils import recarray_fromrecords
In [3]: timeit recarray_fromrecords(rows)
1.88 ms ± 22.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [4]: timeit list(zip(*rows))
72.9 µs ± 483 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

EDIT (MHvK): the change for rows is nice in that it
fixes #8976

@taldcroft taldcroft added this to the v4.0 milestone Aug 24, 2019
@taldcroft taldcroft requested a review from mhvk August 24, 2019 11:25
@taldcroft taldcroft force-pushed the table-rows branch 3 times, most recently from ee06a5c to a5c4c86 Compare August 24, 2019 17:15
mhvk
mhvk reviewed Aug 25, 2019
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the simplifying speedup, and like the ability to just understand list of Quantity, Time. But think I would prefer to just error for lists that can not be easily understood (instead of coercing to object; see in-line comment).

mhvk
mhvk reviewed Sep 14, 2019
8000
View reviewed changes
@mhvk
Copy link
Contributor
mhvk commented Sep 14, 2019

Outstanding issue here is what to do with lists that cannot be understood - turn into object or error.

@mhvk mhvk mentioned this pull request Sep 14, 2019
Closed
@taldcroft
Copy link
Member Author

Rebased and addressed the outstanding issue.

mhvk
mhvk reviewed Sep 14, 2019
View reviewed changes
@taldcroft taldcroft mentioned this pull request Sep 14, 2019
Open
@taldcroft
Copy link
Member Author

Opened #9230 as the promised follow-up issue to talk about object arrays from list.

mhvk
mhvk approved these changes Sep 14, 2019
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: modulo the annoying missing empty line in CHANGES.rst (rebase messes this up too often!), all looks good. Approving so we can merge once that is fixed.

@bsipocz bsipocz merged commit b8bb82b into astropy:master Sep 14, 2019
@bsipocz
Copy link
Member
bsipocz commented Sep 14, 2019

Thank you @taldcroft!

This was referenced Nov 25, 2019
Merged
Closed
eerovaher added a commit to eerovaher/astropy that referenced this pull request Aug 3, 2021
@eerovaher
Remove outdated claims about Table initialization
e98106d 
PR astropy#9165 allowed a `Table` to be initialized from list of rows or list
of dicts containing mixin objects, but some of the relevant
documentation was not updated. This commit removes the obsolete claims.
@eerovaher eerovaher mentioned this pull request Aug 4, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
Performance Ready-for-final-review table
Projects
None yet
Milestone
v4.0
Development

Successfully merging this pull request may close these issues.

Creating table from list of rows fails with numpy-dev (OK for 1.17)
3 participants
@taldcroft @mhvk @bsipocz
0