Skip to content

Adding pandas Int64 support [fix #9280] - #9541

Merged
astrofrog merged 5 commits into
astropy:masterfrom
bmorris3:fix-issue-9280
May 3, 2020
Merged

astrofrog merged 5 commits into
astropy:masterfrom
bmorris3:fix-issue-9280

Conversation

@bmorris3

@bmorris3 bmorris3 commented Nov 6, 2019

Copy link
Copy Markdown
Contributor

Description

This pull request is to address issue #9280. Adding support for pandas nullable integers in the Table.to_pandas function, as suggested by @mar-ses. We're new to contributing to Table, so we're happy to iterate if generating a pandas table from a combination of masked arrays and a pandas Series doesn't jive with the Table style.

Fixes #9280.

@astropy-bot astropy-bot Bot added the table label Nov 6, 2019
@pllim
pllim requested a review from taldcroft November 6, 2019 15:16
@pllim pllim added this to the v4.1 milestone Nov 6, 2019
@pllim

pllim commented Nov 6, 2019

Copy link
Copy Markdown
Member

Thanks!

Test failure seems real, but change log can wait until the relevant section is available.

I'll let subpackage maintainer decides if a new test is necessary.

@taldcroft

Copy link
Copy Markdown
Member

@bmorris3 - my sincere apologies for the late reply. This slipped off my radar and I just noticed it again today.

This looks like a great direction and we should go forward on it. Some issues that need to be addressed are:

  • The current failing test. The current behavior (in master) is that an int MaskedColumn will round-trip through pandas and come back as a masked float. So what appears to be happening in your branch is that it comes back as an Object 'O' dtype. The desired behavior would be perfect round trip where it comes back as an int MaskedColumn.
  • There should be a check on the pandas version to ensure that the installed version is >= 0.24 so that the Int64 type for Series is supported. You'll need to take a little care in testing with regards to the Pandas version since I don't think we have a strict requirements on pandas version for testing.
  • A slight sticky wicket with regards to back-compatibility. We likely need a new keyword argument in to_pandas like use_nullable_int, which would default to None implying use it if possible (pandas version >= 0.24). False would mean never use it and True would mean always use it.

Comment thread astropy/table/table.py Resolved
Comment thread astropy/table/table.py Resolved
@astropy-bot

astropy-bot Bot commented Apr 10, 2020

Copy link
Copy Markdown

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@astrofrog

Copy link
Copy Markdown
Member

@bmorris3 - would you have time to rebase this and implement @taldcroft's comments to get this working?

@taldcroft

Copy link
Copy Markdown
Member

@astrofrog @bmorris3 - I took a quick crack at fixing this. It looks like from_pandas() needs work as well as it is round-tripping back to astropy as an object column. Will see if this is an easy fix or not.

@taldcroft

Copy link
Copy Markdown
Member

Passing tests locally! @astrofrog please have a look and think about what this might break.

@taldcroft
taldcroft requested a review from astrofrog May 2, 2020 18:24
Comment thread astropy/table/table.py
TableReplaceWarning, stacklevel=3)
elif column.dtype.kind in ['f', 'c']:
out[name] = column.filled(np.nan)
out[name] = column

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.

It turns out this intermediate nan-filled copy is not needed, pandas deals with a MaskedArray subclass just fine.

@taldcroft

Copy link
Copy Markdown
Member

@astrofrog -this needs a quick review, or we can just push to 4.2.

@astrofrog astrofrog left a comment

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.

Looks good to me!

@astrofrog
astrofrog dismissed taldcroft’s stale review May 3, 2020 13:39

@taldcroft has now contributed to PR, so approval from different maintainer needed

@astrofrog
astrofrog merged commit 54d7862 into astropy:master May 3, 2020
@bmorris3

bmorris3 commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for carrying this over the finish line, @taldcroft!

@taldcroft taldcroft mentioned this pull request Aug 5, 2021
9 tasks done
eerovaher added a commit to eerovaher/astropy that referenced this pull request Aug 9, 2021
PR astropy#9541 ensured converting a `Table` to a pandas `DataFrame` handles
integer columns, including masked columns, much better, but the
documentation was not updated until now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider using IntegerArrays in to_pandas() for masked int columns

4 participants