ENH: returning (Q)Table for Alma.query_sia #3261
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3261 +/- ##
=======================================
Coverage 69.09% 69.09%
=======================================
Files 232 232
Lines 19637 19642 +5
=======================================
+ Hits 13568 13572 +4
- Misses 6069 6070 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| else: | ||
| result = result.to_table() |
There was a problem hiding this comment.
This will make it backwards compatible.
| else: | |
| result = result.to_table() |
| @@ -464,7 +464,9 @@ def test_sia(): | |||
| sia_mock = Mock() | |||
| empty_result = Table.read(os.path.join(DATA_DIR, 'alma-empty.txt'), | |||
| format='ascii') | |||
| sia_mock.search.return_value = Mock(table=empty_result) | |||
| mock_result = Mock() | |||
There was a problem hiding this comment.
To be backwards compatible, this should work without any changes.
There was a problem hiding this comment.
Yeap, it will all be backwards incompatible as the whole point was to not return pyvo objects but tables.
There was a problem hiding this comment.
OTOH, I expect the only change users need to do is to drop their own to_table, at least that's what I see users were doing for the IRSA method. (Actually, they were doing to_table().to_pandas() most of the time)
There was a problem hiding this comment.
But it's a rather abrupt change. We could use the enhanced_results flag to sway them into that direction, first by introducing it, then giving them a deprecation warning before flipping the flag to on by default. It's less disruptive.
This has been pulled out from #3252. Also, based on the discussion, I added the
enhanced_resultskwarg, but again, this is an API change for whomever is usingquery_siaas they don't get aSIA2Resultsbut a Table/QTable instead.