Skip to content

Make Table inherit primary_key attribute from its parent Table. Fix for [#4672] - #4711

Closed
rirze wants to merge 1 commit into
astropy:masterfrom
rirze:tablepkey
Closed

rirze wants to merge 1 commit into
astropy:masterfrom
rirze:tablepkey

Conversation

@rirze

@rirze rirze commented Mar 19, 2016

Copy link
Copy Markdown
Contributor

This is a fix for issue #4672.

In the issue, running an example from the astropy docs for Table indexing would run into an error. The error was a missing primary_key for the table in question. The error arises when a new Table is made from an existing one, either by slicing or reference, but the primary_key attribute is not transferred to the new Table. This PR fixes that.

I added one test for this case, but I couldn't figure out a way to test for .loc as @taldcroft suggested in the issue. I'm actually not that familiar with Tables or with writing test cases (this is my first attempt to do so), so any feedback would be appreciated.

@rirze rirze changed the title Make Table inherit primary_key attribute from its parent Table. [#4672] Make Table inherit primary_key attribute from its parent Table. Fix for [#4672] Mar 19, 2016
@taldcroft

Copy link
Copy Markdown
Member

@rirze - you should just be able to add tests like:

assert t2.loc[1] == t.loc[1]
 .. etc

Comment thread astropy/table/tests/test_table.py Outdated
t.add_index('a')
original_key = t.primary_key

assert original_key is not None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well test that original_key == ('a',).

@taldcroft

Copy link
Copy Markdown
Member

@rirze - apart from the minor comments this looks good! Please add a bug fix entry to CHANGES.rst in the 1.0.10 section.

@eteq - this should be backported to 1.0.10 and 1.1.x.

@astrofrog

Copy link
Copy Markdown
Member

@rirze - could you rebase this on the latest version of astropy? (there are conflicts at the moment)

@taldcroft

Copy link
Copy Markdown
Member

I just restarted one test that was failing for unrelated reasons. This should be good to merge assuming that passes this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants