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

Fix to generate zero-length copy of current table #4532

Merged
merged 1 commit into from Feb 5, 2016

Conversation

Projects
None yet
5 participants
@anchitjain1234
Contributor

anchitjain1234 commented Jan 26, 2016

For #3742. Added optional max_copy argument to Table.copy method for specifying number of rows to copy.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 26, 2016

Contributor

Does this look OK @taldcroft ?

Contributor

anchitjain1234 commented Jan 26, 2016

Does this look OK @taldcroft ?

@anchitjain1234 anchitjain1234 changed the title from Fix for zero-length copy of current table to Fix to generate zero-length copy of current table Jan 26, 2016

Show outdated Hide outdated astropy/table/tests/test_table.py
assert len(tcopy_1) == len(t)
tcopy_2 = t.copy(max_rows=0)
assert len(tcopy_2) == 0

This comment has been minimized.

@taldcroft

taldcroft Jan 26, 2016

Member

Check here that the column names and types are the same. We're pretty sure this works because slicing works, but since the intent of this PR is to allow for zero-length copying of a table then it should be explicitly checked here.

@taldcroft

taldcroft Jan 26, 2016

Member

Check here that the column names and types are the same. We're pretty sure this works because slicing works, but since the intent of this PR is to allow for zero-length copying of a table then it should be explicitly checked here.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Jan 26, 2016

Member

This also needs a changelog entry for release 1.2.

Member

taldcroft commented Jan 26, 2016

This also needs a changelog entry for release 1.2.

@embray embray added this to the v1.2.0 milestone Jan 26, 2016

Show outdated Hide outdated astropy/table/table.py
out = self.__class__(self, copy=copy_data)
if max_rows is not None:
out = self.__class__(self[:max_rows], copy=copy_data)
else:

This comment has been minimized.

@anchitjain1234

anchitjain1234 Jan 29, 2016

Contributor

@taldcroft If max_rows entered is greater than len(table) than should an exception be raised or default behavior of copying len(table) rows should be allowed?

@anchitjain1234

anchitjain1234 Jan 29, 2016

Contributor

@taldcroft If max_rows entered is greater than len(table) than should an exception be raised or default behavior of copying len(table) rows should be allowed?

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Jan 29, 2016

Member

@anchitjain1234 - sorry, after further discussion with another Astropy core member we decided to not add the max_rows argument after all. We decided to just handle this by documentation, as discussed in #3742. Apologies for the inconvenience.

Doing the documentation-only fix would still be quite useful!

Member

taldcroft commented Jan 29, 2016

@anchitjain1234 - sorry, after further discussion with another Astropy core member we decided to not add the max_rows argument after all. We decided to just handle this by documentation, as discussed in #3742. Apologies for the inconvenience.

Doing the documentation-only fix would still be quite useful!

@mwcraig

This comment has been minimized.

Show comment
Hide comment
@mwcraig

mwcraig Jan 31, 2016

Contributor

Restarted doc build fixed by #4547

Contributor

mwcraig commented Jan 31, 2016

Restarted doc build fixed by #4547

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 1, 2016

Contributor

@taldcroft please check the changes.

Contributor

anchitjain1234 commented Feb 1, 2016

@taldcroft please check the changes.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 3, 2016

Member

@anchitjain1234 - the windows doctest is failing because windows defaults to int32. You should write the example using floats, which will be float64 on all platforms. Otherwise 👍.

Member

taldcroft commented Feb 3, 2016

@anchitjain1234 - the windows doctest is failing because windows defaults to int32. You should write the example using floats, which will be float64 on all platforms. Otherwise 👍.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 3, 2016

Contributor

@taldcroft Does this documentation fix needs a changelog entry?

Contributor

anchitjain1234 commented Feb 3, 2016

@taldcroft Does this documentation fix needs a changelog entry?

Fixed construct_table documentation to include example for creating c…
…opy of existing table that is empty. Fixes #3742
@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 3, 2016

Member

@anchitjain1234 - no changelog entry is required.

Member

taldcroft commented Feb 3, 2016

@anchitjain1234 - no changelog entry is required.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 4, 2016

Contributor

@taldcroft Is any other change needed in this PR?

Contributor

anchitjain1234 commented Feb 4, 2016

@taldcroft Is any other change needed in this PR?

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 4, 2016

Member

@anchitjain1234 - it's probably OK, but I just restarted one travis test case that had failed probably for unrelated reasons. Once that passes this should be good to merge.

Member

taldcroft commented Feb 4, 2016

@anchitjain1234 - it's probably OK, but I just restarted one travis test case that had failed probably for unrelated reasons. Once that passes this should be good to merge.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 5, 2016

Contributor

@taldcroft Could it be merged now.

Contributor

anchitjain1234 commented Feb 5, 2016

@taldcroft Could it be merged now.

@taldcroft taldcroft self-assigned this Feb 5, 2016

taldcroft added a commit that referenced this pull request Feb 5, 2016

Merge pull request #4532 from anchitjain1234/fix-3742
Documentation update for generating a zero-length copy of current table

@taldcroft taldcroft merged commit 6e14021 into astropy:master Feb 5, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 76.756%
Details
@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 5, 2016

Member

Merged, thanks @anchitjain1234 !

Member

taldcroft commented Feb 5, 2016

Merged, thanks @anchitjain1234 !

@anchitjain1234 anchitjain1234 deleted the anchitjain1234:fix-3742 branch Feb 5, 2016

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