Repository navigation
DOC: add documentation on why astropy has tables - #17520
Conversation
|
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.
|
taldcroft
changed the title
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
I like the text very much. My main question is whether this is not better as a separate page, as in the main text it seems a bit distracting. A separate page would also more easily allow (others) to add more detailed information about transformations between QTable and pandas. Maybe a new interoperability.rst?
taldcroft
changed the title
|
@mhvk - good idea with the new page. I also took the opportunity to make this more positive at the top level. More of a "Why astropy table" rather than "Why not pandas", along with the relationship between the two. Also added a mention of polars at the end. |
|
@mhvk - I also substantially updated the page on |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This looks great! I found a small typo, but otherwise I think we should just get it in.
|
Thanks for the quick review @mhvk. Auto-merge is enabled. |
|
@mhvk - this is failing tests because of the new functions |
|
Patched in #17523 |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
I have some comments, mostly for clarification.
| - :ref:`Time formats <time-format>` used in astronomy such as the FITS time format are | ||
| not supported. | ||
|
|
||
| Pandas does not support coordinate columns. |
There was a problem hiding this comment.
this is significant, maybe you want to divide this section into two, one for time and one for coordinates?
|
btw, just a small suggestion: if review is requested over the weekend it would be a courtesy of not setting up auto-merge before that very same weekend ended. |
|
I disabled auto merge for now since there are new comments. FYI. |
278d9bb to
f20ab4b
Compare
| example, accessing metadata like units is a key component of using the | ||
| `astroquery <https://astroquery.readthedocs.io/en/latest/>`_ service for astronomical | ||
| data analysis. |
There was a problem hiding this comment.
I still don't get the astroquery reference, and why this is a key component for astroquery. We could return whatever comes back from the server, typically a VOTable or csv or whatever text file object. It's just a compatibility thing that we went a step further and parse the results into an astropy Table.
(and returning pandas definitely brought up by a couple of modules but we pushed them to do astropy Tables instead to be more consistent with the older modules).
There was a problem hiding this comment.
IMO a key feature of astroquery is exactly the hard work of parsing heterogenous results from data providers into a uniform and predictable data structure (QTable) that is easy for users to work with. Many of those datasets include physical quantities with units, column descriptions, or other metadata, so if astroquery returned a DataFrame then that information would be lost.
That said, if you really don't want this shout-out to astroquery then I can replace it with a different example.
There was a problem hiding this comment.
Seconded - for me part of the beauty of astroquery is exactly that it produces a table with usable columns!
There was a problem hiding this comment.
Yes, I think if you can go with a different example it would be OK as I'm just not comfortable with the
is a key component of using the astroquery service for astronomical data analysis summary, and nitpicking the sentence away is not very constructive.
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Thanks! Some comments below but overall looks good.
|
OK all the @pllim comments are addressed, I think. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Reread, and like it very much still! One suggestion for the stop, to make the pandas reference just one item.
| * :ref:`Read and write <read_write_tables>` to files via the :ref:`Unified File Read/Write Interface <table_io>`. | ||
| * Convert to and from `pandas.DataFrame`. | ||
|
|
||
| The :ref:`astropy-table-and-dataframes` page provides the rationale for maintaining |
There was a problem hiding this comment.
I'd move this as part of the item just above -- maybe
* Convert to and from `pandas.DataFrame` (see :ref:`pandas`
as well as :ref:`astropy-table-and-dataframes` for the rationale
for a dedicated `astropy.table` package instead of relying on `pandas`).
(not sure I got the references quite right - want to have a direct link to pandas.rst as well...)
p.s. Fine to ignore!
There was a problem hiding this comment.
Looking at the rendered HTML again, I saw that every list item was succinct and fit on one line, with the exception of the one about mixins. So instead of making the pandas line longer, I shortened the mixin line and now it looks very clean.
This of course leaves the part about astropy-table-and-dataframes as a distinct coda to the introduction, but this comes up so often that it seems OK.
Co-authored-by: P. L. Lim <[email protected]>
132533e to
2ab135a
Compare
…520-on-v7.0.x Backport PR #17520 on branch v7.0.x (DOC: add documentation on why astropy has tables)
Description
This pull request is to address a common question of why astropy maintains its own Table class instead of using pandas DataFrame.
It also tightens up the opening description and feature list.
This also substantially updates the page on
Interfacing with the Pandas Package. Some of that was out of date or just wrong, so I fixed things and added a detailed example highlighting known issues.Fixes #17146