Skip to content
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

Jump to bottom

Allow specifying column names in Table.as_array() #8532

Merged
merged 4 commits into from Apr 10, 2019
Merged

Allow specifying column names in Table.as_array() #8532

merged 4 commits into from Apr 10, 2019

Conversation

geekypathak21
Copy link
Contributor

@geekypathak21 geekypathak21 commented Mar 28, 2019 •
edited

#8529 @taldcroft Please have a look at this PR.

  • Test for key_values is added.
  • keyword added in Table.as_array()

@bsipocz bsipocz added this to the v3.2 milestone Mar 29, 2019
@bsipocz bsipocz added the table label Mar 29, 2019
@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #8532 into master will decrease coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8532      +/-   ##
==========================================
- Coverage   86.81%   86.81%   -0.01%     
==========================================
  Files         386      386              
  Lines       58164    58169       +5     
  Branches     1060     1060              
==========================================
+ Hits        50495    50497       +2     
- Misses       7054     7057       +3     
  Partials      615      615
Impacted Files Coverage Δ
astropy/table/table.py 93.33% <85.71%> (-0.07%) ⬇️
astropy/utils/data.py 80.93% <0%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e3af0...02d864c. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 29, 2019 •
edited

Codecov Report

Merging #8532 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8532      +/-   ##
==========================================
- Coverage   86.81%   86.81%   -0.01%     
==========================================
  Files         386      386              
  Lines       58162    58164       +2     
  Branches     1060     1060              
==========================================
+ Hits        50493    50494       +1     
- Misses       7054     7055       +1     
  Partials      615      615
Impacted Files Coverage Δ
astropy/table/table.py 93.49% <100%> (+0.01%) ⬆️
astropy/samp/hub.py 77.38% <0%> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8824789...a32c099. Read the comment docs.

@geekypathak21
Copy link
Contributor Author

@bsipocz I think Tarvis-ci failure is not related to this PR.

taldcroft
taldcroft requested changes Apr 5, 2019
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Thanks! A few changes needed but this is mostly there.

astropy/table/table.py Outdated Show resolved Hide resolved
astropy/table/table.py Outdated Show resolved Hide resolved
astropy/table/table.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@taldcroft taldcroft changed the title Idea about column slicing Allow specifying column names in Table.as_array() Apr 5, 2019
@geekypathak21
Copy link
Contributor Author

@taldcroft Thanks for reviewing changes performed as suggested

taldcroft
taldcroft reviewed Apr 10, 2019
View reviewed changes
astropy/table/table.py Outdated
@@ -261,7 +265,11 @@ def as_array(self, keep_byteorder=False):

dtype = []

cols = self.columns.values()
if names != None:
cols = self[names].columns.values()
Copy link
Member

Choose a reason for hiding this comment

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

@himanshupathak21061998 - sorry, I made this comment a few days ago but must not have actually put it it in the review... So one more fix, related to performance. Using self[names] is simple and clean, but it is inefficient since this makes a temporary full copy of all the data for names and creates a new table just to get out the subset of columns. Instead:

cols = self.columns.values()

if names != None:
    cols = [col for col in cols if col.info.name in names]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taldcroft Thanks for reviewing suggested changes have been performed. Yes I didn't notice this comment

taldcroft
taldcroft approved these changes Apr 10, 2019
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks great @himanshupathak21061998, thanks!!

@taldcroft taldcroft merged commit 1714169 into astropy:master Apr 10, 2019
@geekypathak21
Copy link
Contributor Author

Thanks for merging

@bsipocz bsipocz mentioned this pull request Apr 11, 2019
Closed
@geekypathak21 geekypathak21 deleted the table-as_array branch April 14, 2019 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taldcroft taldcroft taldcroft approved these changes

Assignees
No one assigned
Labels
table
Projects
None yet
Milestone
v3.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@geekypathak21 @taldcroft @bsipocz