Euclid: upgrade the TAP method upload_table to upload tables in fits format #3295
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3295 +/- ##
==========================================
+ Coverage 69.36% 69.82% +0.46%
==========================================
Files 232 232
Lines 19692 19711 +19
==========================================
+ Hits 13659 13764 +105
+ Misses 6033 5947 -86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
While the PR looks good as is, I run into a few concerns by looking at the diff. Most of these are due to the fact that this code is old and could probably use some cleaning-up/refactoring.
And the other part is the lack of tests reaching out to the server; I would definitely love to see more of those but they are not a blocker for this PR.
And one minor last comment, that is terribly nitpicky: there are a lot of commit with the same commit message. Could you consider squashing these?
| "FORMAT": str(resource_format)} | ||
| "FORMAT": 'votable'} |
There was a problem hiding this comment.
Can we start adding tests that reaches the server? While the changes in this PR look good I have the sense that the previous version might not have worked as intended.
Getting some remove coverage would boost my confidence that all works as assumed :)
astroquery/utils/tap/core.py
Outdated
| table description | ||
| format : str, optional, default 'VOTable' | ||
| format : str, optional, default 'votable' | ||
| resource format |
There was a problem hiding this comment.
you want want to add a sentence here of what formats are accepted; here and everywhere else below
| def __uploadTableMultipart(self, resource, *, table_name=None, | ||
| table_description=None, | ||
| resource_format="VOTable", | ||
| def __uploadTableMultipart(self, resource, *, table_name=None, table_description=None, resource_format="votable", |
There was a problem hiding this comment.
Not a big deal as this is all private, but resource_format is kind of ignored in a few cases, and is contradictory with resource.
astroquery/utils/tap/core.py
Outdated
| chunk = f.read() | ||
| files = [['FILE', os.path.basename(resource), chunk]] | ||
| else: | ||
| table = Table.read(str(resource)) |
There was a problem hiding this comment.
does this work out of the box? Table usually requires the format kwarg or goes into guessing rabbit holes.
Also, are we expecting complex votables to be uploaded?
Anyway, all these look to be a bit too complicated with the logic of reading in resources then writing them out and then reading them in again to upload. I trust your oversight here to know what is going on in the different schenarios and that all necessary cases are covered, but if there is a way to simplify it, I would love to review and merge that PR.
cosmoJFH
force-pushed
the
ESA_euclid_EUCLIDPCR-1971_upload_user_table_from_fits_file
branch
2 times, most recently
from
550eb84 to
54fe088
Compare
To use the method upload_table you need to log in the archive, since the method uploads the table to the user's private area. I don't know how to design remote tests in this case.
Sorry for this. I have squashed the latest commits. |
…kes use of the format and the documentation of the parameter format has been improved.
OK, we indeed don't have anything centrally worked out for dummy authentication, but adding tests for the publicly accessible functionality would still be great.
sadly something went wrong, some commits (with the same content) are now in the history multiple times. I would guess you didn't force push the changes back but added a merge commit instead. Anyway, I'll just go ahead and rebase/cleanup and merge this now. |
| if c.name == column_name: | ||
| def __find_column(columnName, columns): | ||
| for c in (columns): | ||
| if c.name == columnName: |
There was a problem hiding this comment.
I suppose this got reverted to the previous camelcase in the rebase gone wrong, in the newly rebased version this is being fixed now.
bsipocz
force-pushed
the
ESA_euclid_EUCLIDPCR-1971_upload_user_table_from_fits_file
branch
from
01d570f to
868c135
Compare
2 participants
Dear Astroquery team,
the Euclid documentation (https://astroquery.readthedocs.io/en/latest/esa/euclid/euclid.html#uploading-table-from-file) includes the following information for the method upload_table: "A file containing a table (votable, fits or csv) can be uploaded to the user’s private area", but we have checked that only votable files can be uploaded, otherwise an exception is thrown.
So, we would like to upgrade the TAP method upload_table to upload files in fits format (and other formats), making use of an astropy Table: only a file associated to any of the formats described in
https://docs.astropy.org/en/stable/io/unified.html#built-in-table-readers-writers, and automatically identified by its suffix
or content could be used.
This fix would also solve #2529
cc @esdc-esac-esa-int
jira: EUCLIDPCR-1971