Skip to content

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

Merged
bsipocz merged 4 commits intoastropy:masterfrom
taldcroft:table-rows
Sep 14, 2019
Merged

Allow adding a table column as a list of mixin-type objects (+ row improvement)#9165
bsipocz merged 4 commits intoastropy:masterfrom
taldcroft:table-rows

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

taldcroft commented Sep 14, 2019

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

taldcroft commented Sep 14, 2019

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