feat: Generic Table Conversion through Narwhals - #18435

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@TheSkyentist - thanks for this draft PR, it looks excellent and a significant step toward better interoperability! Here is a quick response to your first two questions, more to come...

• About pandas - as you know astropy is mature and has a large user base and we are very sensitive to API changes.

  • Pandas should remain the default for now.
  • Deprecating to/from_pandas would have a high impact in terms of code disruption and we'd prefer to avoid that. The downside is ending up with two ways to do the same thing, but see below.

• Dependencies - as a follow-on to stability, my first thought would be essentially leaving the existing to/from_pandas as-is:

  • Maintains the ability to convert with only pandas as a dependency. I think this is important.
  • Maintains the same compatibility handling and exception messages (e.g. time, handling a multidimensional column).
  • Again, not the most perfect from a code perspective, but this strategy prioritizes the user experience.

I think this is great advice. I've gone ahead and added the to/from pandas methods back to maintain compatibility. I've also added import warnings to the new methods.

Nullable integers - as a general rule the priority goes to maintaining existing tests since (in practice) they define the behavior of astropy more than the documentation. So here you should revert to so the original test passes and fix the documentation.

Error Handling - when possible the goal is to catch common/expected exceptions before they happen downstream and raise exceptions with helpful suggestions.

I'm wondering now if it might make sense to make a class-based implementation that allows for backend-specific handling of the interface issues. I.e. a class with a method to validate the input columns for to_df() based on the backend. This might also handle checking optional dependencies and so forth. We would still allow any backend that narwhals supports but would be leaving it to the user to ensure that things work and/or understand problems.

It would then be good to make a distinction in the docs between dataframe types that are explicitly tested vs. those that are not.

I'm also thinking there should be a new module e.g. dataframes.py that has all of this code (including the legacy to/from_pandas functions, where the Table methods just become a thin wrapper around those).

I have updated the behaviour with use_nullable_int=False to raise an error if there are masked integer columns. It's the same result as before but with a little cleaner error handling and now reports the offending column.

This will definitely deserve a What's New section!

@TheSkyentist - FYI I'll be on vacation July 16-26 so I won't be responding during that time.

Thank you for your feedback already! I do think being explicit about the tested dataframes is a good idea. It's worth thinking about how best to place this code within the Astropy Table ecosystem and do rigorous input checks. I think an entire module might be overkill, at least while the Astropy Table isn't attempting to be fully DataFrame compliant.

I'm happy to discuss more after you get back from your vacation and hold of on major changes until then.

Side note, how do I add a a What's New beyond the changes.rst?

Re: What's New -- It is not managed by towncrier; rather one manually adds a narrative section in docs/whatsnew/7.2.rst. You can see previous releases versions for inspiration.

Thank you!

@TheSkyentist - about the separate module, the table.py module is already way too long at over 4000 lines, so I think that trying to factor out functionality (especially new code) is a good thing. Module import is pretty cheap, especially since it could be local to the to/from methods. In the end with some of the ideas here dataframes.py won't be so small. 😄

The biggest hassle I notice with factoring out methods to the actual work functions is duplicate docstrings. I'm not sure if there are clean and accepted ways to handle this.

Have some time to work on this again, not quite ready for merge, still need to fix up and debug the tests, but I've refactor the code into dataframes.py (and test_df.py) and expanded the documentation. It would be great to get feedback at this stage as I continue to make sure all behaviour is as expected in the test suite.

All tests passing! The only feature I haven't managed to get working yet is 1D arrays as entries when converting to/from PyArrow tables, but multidimensions support works for Polars. I will try this a little longer but, if it remains elusive, I think we can leave it for a future PR. Currently the additional features already satisfy the original purpose.

I will work on the What's New next.

I've also rebased to make merging easy.

I think we are ready for review and potential merge :-)

Only test failing is with the oldest supported versions of all dependencies as Narwhals conversion with PyArrow requires PyArrow >= 11 whereas the astropy has a minimum version of 10. Bumping PyArrow would likely solve this. Is this desired?

Bumping PyArrow would likely solve this. Is this desired?

If https://pypi.org/project/pyarrow/#history is the release history you are referring to , then I think it is okay since 11.0.0 was released on 2023-01-27, which is more than 2 years ago.

Hmm still failing with pyarrow 11

A little trial and error on my part find that the tests pass with PyArrow >= 18. About nine months ago in terms of release date.

I went through and made the appropriate changes to the testing suite :-)

@neutrinoceros It's been a pleasure! It's been a great experience contributing to Astropy and hopefully it won't be the last time! I appreciate everyone's hard work on getting this PR production ready and I've learned a lot along the way :-)

Why was the type check that backend is a str removed? This leads to what I consider a bug, given the documentation and our agreement that only strings are allowed.

In [4]: import pandas as pd

In [5]: t.to_df(pd)
Out[5]: 
   a    b  c
0  1  1.0  c
1  2  2.0  d
2  3  3.0  e

@TheSkyentist - I'd like to ask you to revert a change requested by @neutrinoceros which was renaming dataframes.py to _dataframes.py.

There is some discussion on this, but the overriding policy at this time is to maintain the existing style and conventions of a sub-package. For table, none of the Python modules are named with a leading _. Doing so now for a single module introduces confusion about what that implies, and probably does not prevent people from using that module anyway, e.g.

    # Using private API while narwhals-dev/narwhals#3150 is not merged
    if not nw._utils.is_eager_allowed(backend_impl):

I fear this would set a dangerous precedent that we cannot ever mark any module as explicitly private: adding the leading _ after the fact is technically a breaking change, which I believe is the main argument against moving existing modules. Preventing new modules from being marked in the name of consistency with our existing (known problematic) ways, is not, in my humble opinion, doing anyone a favor. You still have final authority here, of course, but I feel this needs be said.

I'll hold of on the renaming of the dataframes module, but to the point on type checking for backend.

Currently backend is type checked as a string, so this would be caught by a linter/debugger. The reason for the more explicit catch/except before was that I was not properly using the Narwhals typing system so the previous backend argument only had a type of "Any" which cannot help in debugging.

I think type-hinting and express statements in the documentation should be sufficient, i.e. we don't expressly type check all other inputs/arguments.

I fear this would set a dangerous precedent that we cannot ever mark any module as explicitly private

At some level the precedent has already been set by 14 years of development, as there are just 11 "private" modules out of 841 in astropy, and Pandas is similar.

The question is what is really being gained, especially in context of overwhelming precedent. The main reason I would want to mark an entire module private is if it might credibly disappear entirely in the future or need to be moved or renamed, but I think dataframes.py is here to stay. Barring that, if all the functions in dataframes should be private then they should be named _* and not included in __all__ (which also sends to me a strong message of "public", regardless of the module name).

Currently backend is type checked as a string, so this would be caught by a linter/debugger.

I think that, to one digit of precision, about 0% of astropy users would catch an error in that way (myself included). 😄

I think type-hinting and express statements in the documentation should be sufficient, i.e. we don't expressly type check all other inputs/arguments.

Sure. I worry a little about folks with just a bit of deeper knowledge that figure out that other types actually work and start putting that in code. But I guess that is their problem.

In any case, since we can indeed revert _dataframes.py to dataframes.py at any time, I'm just going to merge this now for a happy Friday!

Congratuations @TheSkyentist on your first PR and a very substantial and important one at that!

The question is what is really being gained

Simple: the ability to change our mind if we ever need to

if all the functions in dataframes should be private then they should be named _*

It's not so much about the status of the functions themselves, rather, that of the namespace.

which also sends to me a strong message of "public"

While true, this only works for readers of downstream code who care enough to either read the module's source, or use type checkers (or linters) that might of unincluded functions as private. Either way it's a minority. Marking the namespace itself as private makes it less likely that someone comes to us with complains that we pulled a rug under them we didn't know they'd rely on. I'd argue if you feel strongly about dropping the underscore, it would be extremely easy to do as a follow up PR so I'd advise that we defer this point. What do you think ?

Oh, I got there too late apparently. Well, at least we converged exactly where I hoped to !
Congratz @TheSkyentist, I hope to see more of you in the future !

Should not have skipped doctest blindly, see #19716