Repository navigation
Accessing Column.quantity always performs expensive deepcopy operation #14244
Description
Description
Accessing astropy.table.Column.quantity always performs an expensive deepcopy of the .info object, although this information can't even be accessed on temporary objects:
In [19]: t['a'].quantity.info
...
AttributeError: failed to access "info" attribute on a temporary object.
So something that is expensive is done also in cases where it can never actually be usefull, e.g. in situations like:
c = t['a'].quantity + b
Expected behavior
Perform expensive operations only when actually needed.
Actual behavior
DataInfo.__set__ is called which performs expensive, not needed operations.
Steps to Reproduce
In [1]: import astropy.units as u
In [2]: from astropy.table import Table
In [3]: t = Table({"a": [1, 2, 3] * u.deg})
In [4]: b = [1, 2, 3] * u.deg
In [5]: %prun -s tottime t['a'].quantity + b
170 function calls (166 primitive calls) in 0.000 seconds
Ordered by: internal time
ncalls tottime percall cumtime percall filename:lineno(function)
1 0.000 0.000 0.000 0.000 {built-in method builtins.exec}
2 0.000 0.000 0.000 0.000 data_info.py:373(__set__)
8 0.000 0.000 0.000 0.000 copy.py:128(deepcopy)With timings:
In [9]: %%prun -s tottime
...: for i in range(100000):
...: t['a'].quantity + b
...:
16700003 function calls (16300003 primitive calls) in 11.563 seconds
Ordered by: internal time
ncalls tottime percall cumtime percall filename:lineno(function)
800000 1.390 0.000 2.634 0.000 copy.py:128(deepcopy)
200000 1.182 0.000 5.392 0.000 data_info.py:373(__set__)
300000 0.674 0.000 6.732 0.000 quantity.py:584(__array_finalize__)
100000 0.554 0.000 4.729 0.000 quantity.py:434(__new__)
100000 0.548 0.000 5.821 0.000 quantity.py:620(__array_ufunc__)
System Details
Linux-5.14.0-1051-oem-x86_64-with-glibc2.35
Python 3.9.15 | packaged by conda-forge | (main, Nov 22 2022, 08:45:29)
[GCC 10.4.0]
Numpy 1.23.5
pyerfa 2.0.0.1
astropy 5.2
Scipy 1.9.3
Matplotlib 3.6.2
Activity
mhvk commentedon Jan 2, 2023
It is not really a bug, and certainly the equivalent is used for changing columns from a
Tableto quantities in aQTable. But I can see it is annoying. In principle, I guess one could change the implementation toQuantity(col.data, unit=col.unit, dtype=col.dtype, copy=False, order="A", subok=True)-- it is unlikely there are many cases where people rely on.infobeing set when they docol.quantity.I wonder, though, why this comes up, as the presence of
.quantitywas really most to work-around the fact thatQuantitycould originally not be part of aTable.QTabledoes not have this restriction; is there a reason not to use that instead?maxnoe commentedon Jan 3, 2023
We had some issues related to metadata access when using pure tables, not QTable, let me dig that up. If using
QTablesolves that, we should probably just switch to usingQTable.maxnoe commentedon Jan 3, 2023
Using a qtable is still somehow slower than using the pure table column, although the units are handled correctly and a quantity is returned by the pure column operation.
mhvk commentedon Jan 3, 2023
But in a pure table, you presumably have an actual
Column, so the unit is ignored, which would naturally make it faster.maxnoe commentedon Jan 3, 2023
No, the unit is not ignored, it also works with the column in deg and the other quantity in rad, giving the correct result.
mhvk commentedon Jan 3, 2023
Ah, I see, so
bis aQuantity. I checked and the difference is actually related to this issue:qt['a'] + bcopiesinfoand stores it on the result. Which seems a bug: after the addition, thename, etc., really shouldn't be kept! See #14248 and actually also the much older #5047.mhvk commentedon Jan 4, 2023
So at least the slowdown you found in #14244 (comment) should be addressed by #14253
mhvk commentedon Jan 5, 2023
#14253 is merged. Hopefully this removes one disadvantage of
QTable!mhvk commentedon Jan 6, 2023
Coming back to the topic of this PR, I think that in doing
col.quantitya user really would not generally expect that the name and description of a column would be preserved, so that it would be fine to change the implementation to be the fasterQuantity(col.data, unit=col.unit, dtype=col.dtype, copy=False, order="A", subok=True)I suggested above. My only question really is whether it is worth it... Let me ping @taldcroft for suggestions.