Added support for QTable #384
Conversation
Codecov Report
@@ Coverage Diff @@
## main #384 +/- ##
=======================================
Coverage 78.55% 78.56%
=======================================
Files 47 47
Lines 5564 5565 +1
=======================================
+ Hits 4371 4372 +1
Misses 1193 1193
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
On Wed, Nov 02, 2022 at 01:13:03AM -0700, Brigitta Sipőcz wrote:
@bsipocz commented on this pull request.
I still wonder why not change `to_table` to return a QTable?
I declare being a solid non-expert here. If you're reasonably sure
Table-expecting code won't break when faced with QTable-s and they're
about the same in terms of CPU, I'd be swayed to just changing the
return type, perhaps noting in the docs that the extra QTable
features can only be used in pyVO 1.5 or later (or so).
|
QTable is a subclass of Table, so it all should be fine. There is certainly an overhead of creating the mixin columns, but I don't think it would be a significant factor for the use cases here. However, one issue could be is there are columns with non-valid units, those may throw warnings or are rendered as unknown. But IMO those are all upstream issues, ideally, VO services return valid VO units only. |
|
I'm pinging @taldcroft here, he can comment more on the nuances. |
|
@bsipocz the return types are not compatible anymore and might break existing client code. For example: |
|
on the other hand, all of those |
|
On Wed, Nov 02, 2022 at 09:34:15AM -0700, Adrian wrote:
@bsipocz the return types are not compatible anymore and might break existing client code. For example:
```
res.to_table()['s_ra'][0] > 1
True
res.to_qtable()['s_ra'][0] > 1
{UnitConversionError}Can only apply 'greater' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)
```
Ouch. That nails it for me. Let's have to_qtable() next to
to_table(). I won't argue against reworking examples to nudge people
towards to_qtable, but having to methods is so cheap that we
shouldn't break code that's arguably not unreasonable.
|
I would argue that this is exactly the case of an unreasonable code, after all, space probes got lost due to false unit assumptions. If we must support such a use case, I would make it opt-in, e.g. adding a Also, I don't share the notion that methods are cheap. Adding more methods is complexifying the API which in the long run leads to not so great user experience (think of the mpl or pandas API). |
|
@bsipocz I see your point and I agree with you but we can't just break user code without giving them a warning at least. They might already handle the units by looking at the unit of the column in their code. Other units like We could add |
I like this idea as a good middle ground. |
|
These are just convenience methods which should encourage (make it easy) to do things the right way. It is still easy to get a simple I'll proceed with the changes if we are all in agreement. |
|
Since I was looped in, I'll express my opinion that the |
|
I think the reason not to return QTable back then was that a service may return invalid units |
It's probably still the case sometimes. Warnings are issued when the value cannot be parsed and that could be annoying enough for users to push the service providers to fix them. |
|
@bsipocz - how should proceed with this? It sounds like the question now is whether to deprecate |
|
I can't recall any serious limitations of a QTable as opposed to a Table in the contect of VO, or table sizes served by VO (as opposed to big data tables read in from parquet, or being process with spark or dask, for those I don't think either Table or QTable would work anyway, but one should go with a flavour of dataframes and forget about astropy and units and etc), so personally I would deprecate it rather than having both. |
|
That documentation fix sounds ok to me but I'm really peripheral here. |
|
I'd rather have it added to the API than just mentioning it into the docs. One, because I think it's the better method (and was requested by our users) and two because the IDE will always show it as an option as opposed to having to remember about the conversion method from the docs. |
|
Yes, I think it's also the better option, and in the context of pyvo folks should use qtables. the only thing I don't like is to have both. But it's not a big deal. |
|
OK, I prefer consensus but if there are no vetos I would need to merge this. |
#381