Add new QTable kwarg and attribute preserve_quantity_dtype - #20247

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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@taldcroft - before I review, a question about best approach: I dislike integer Quantity generally, but it seems especially bad to use it if a column is not meant to be a Quantity, which is the case here.

The most targeted approach would seem to be to simply fix votable, removing the unit if it is equal to "NA", since the intent is super clear: the unit is "Not Applicable".

Or, since that would affect round-tripping, we can still create an UnrecognizedUnit of that name, but refuse to convert to Quantity if the unit is "not applicable".

Or, one could be a bit stricter, and refuse to convert for any unrecognized unit (with maybe an option to force conversion).

Though at that point, I'd maybe tend to also not convert columns with exact dtype if the unit is dimensionless. After all, dimensionless will work correctly with quantities anyway, as that is the default assumed unit.

@mhvk - I think here we really need to avoid the temptation to guess, for instance guessing that "NA" means Not Applicable. Any kind of guessing is bound to lead to surprise for some user (esp. non-English speaker).

The most straightforward and consistent principle to apply rigorously is round tripping. Here is an example with ECSV but the same applies for FITS, VOtable, HDF5 (any format that supports rich metadata).

q0 = u.Quantity([1, 2], dtype=int, unit=u.def_unit("NA"))
q1 = u.Quantity([3, 4], dtype=int, unit="ct")
q2 = u.Quantity([5, 6], dtype=int, unit=u.dimensionless_unscaled)
qt = QTable([q0, q1, q2])
qt.info

<QTable length=2>
name dtype unit  class  
---- ----- ---- --------
col0 int64   NA Quantity
col1 int64   ct Quantity
col2 int64      Quantity

out = io.StringIO()
qt.write(out, format="ecsv")
print(out.getvalue())

# %ECSV 1.0
# ---
# datatype:
# - {name: col0, unit: NA, datatype: int64}
# - {name: col1, unit: ct, datatype: int64}
# - {name: col2, datatype: int64}
# meta: !!omap
# - __serialized_columns__:
#     col0:
#       __class__: astropy.units.quantity.Quantity
#       unit: !astropy.units.Unit {unit: NA}
#       value: !astropy.table.SerializedColumn {name: col0}
#     col1:
#       __class__: astropy.units.quantity.Quantity
#       unit: !astropy.units.Unit {unit: ct}
#       value: !astropy.table.SerializedColumn {name: col1}
#     col2:
#       __class__: astropy.units.quantity.Quantity
#       unit: !astropy.units.Unit {unit: ''}
#       value: !astropy.table.SerializedColumn {name: col2}
# schema: astropy-2.0
col0 col1 col2
1 3 5
2 4 6

The ECSV file above is an unambiguous requirements document of what the output table should look like.

qt2 = QTable.read(out.getvalue(), format="ecsv")
qt2.info

Wrong dtype! The UnitsWarning is appropriate as a heads-up that astropy is surprised. Nevertheless the output QTable should exactly match what the ECSV file specified.

WARNING: UnitsWarning: 'NA' did not parse as unit: At col 0, NA is not a valid unit. Did you mean nA, nBa, nPa or na? If this is meant to be a custom unit, define it with 'u.def_unit'. To have it recognized inside a file reader or other code, enable it with 'u.add_enabled_units'. For details, see https://docs.astropy.org/en/latest/units/combining_and_defining.html [astropy.units.core]
<QTable length=2>
name  dtype  unit  class  
---- ------- ---- --------
col0 float64   NA Quantity
col1 float64   ct Quantity
col2 float64      Quantity

The most targeted approach would seem to be to simply fix votable, removing the unit if it is equal to "NA", since the intent is super clear: the unit is "Not Applicable".

If the VOtable standard formally defines "NA" as the unit being "not applicable", then dropping the unit upstream in votable.connect might be appropriate. But that is out of scope, and does not change the need for keeping the int dtype of other valid (and quite common) units like ct (count). Again, if the VOtable file says the dtype is int, the output should reflect that.

Or, one could be a bit stricter, and refuse to convert for any unrecognized unit (with maybe an option to force conversion).

Since astropy provides a convenient way to create quantities with unrecognized units, we should support round-tripping them by default. The existing warning seems sufficient to me.

At a more fundamental level, I find this behavior inconsistent and surprising:

>>> c1 = Column([1, 2], dtype=int, unit="ct")
>>> c2 = u.Quantity([1, 2], dtype=int, unit="ct")
>>> qt2 = QTable([c1, c2])
>>> qt2.info
<QTable length=2>
name  dtype  unit  class  
---- ------- ---- --------
col0 float64   ct Quantity
col1   int64   ct Quantity

This would almost argue for making preserve_quantity_dtype=True be the default, but of course this might be too much breakage.

OK, agreed that really there are two separate concerns, one generally about integer dtype with presumably mostly a unit where that makes sense (like your ct), and one about units that are explicitly set to mean not-applicable.

Indeed, I was getting convinced that perhaps the default should be to keep dtype consistent, until I tried,

q = u.Quantity(np.arange(10), "ct", dtype=int)
q /= 10*u.s
# TypeError: Arguments cannot be cast safely to inplace output with dtype=int64

while with auto-conversion to float it succeeds,

q = u.Quantity(np.arange(10), "ct")
q /= 10*u.s

which brings back my confidence that quantities should really be float by default. (Of course, out-of-place is always OK)

Anyway, in the absence of a clear best way, your option of giving choice does seem best. I'll try to have a look at the actual PR later.

Numpy has the same behavior, so users really should not be surprised. Indeed, in-place auto-conversion would be the real surprise:

In [1]: x = np.array([1, 2, 3])

In [2]: x /= 5.3
---------------------------------------------------------------------------
UFuncTypeError                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 x /= 5.3

UFuncTypeError: Cannot cast ufunc 'divide' output from dtype('float64') to dtype('int64') with casting rule 'same_kind'

And Quantity does the right casting to float when expected:

In [4]: q = u.Quantity([1, 2], dtype=int)

In [5]: q / (5 * u.m)
Out[5]: <Quantity [0.2, 0.4] 1 / m>

BTW, I'm not saying that u.Quantity([1, 2], unit='m') should give an int dtype because that would be a huge API break at this point. Your example hinted at changing Quantity itself?

I was only talking at the QTable level with making preserve_quantity_dtype=True as the default. We'd need to consider this carefully because it would certainly change the dtype for some real world data (but arguably for the better).

Yes, maybe we can think about whether there would be some path towards keeping read-in dtypes as they are. This might be more inside the QTable.read and QTable(table) interfaces (should really look at your PR before commenting further, but several meetings to go first...).

Does this supersede #12505 ?

It will if it is merged. But honestly #12505 is very unlikely to get merged at this point.

@mhvk - the actual code changes are quite simple... 2 new lines, 1 changed line and 1 moved line (thanks to TableAttribute!) .Of course there is everything else that goes with it, but I think the conceptual discussion is the hard part here.

@mhvk - will do on the other comments, thanks.

@mhvk - Here is a completely different idea for consideration. It seems cleaner to me and also brings a new possibility for Quantity.

Add a new configuration variable for units named (e.g.) quantity_convert_int_to_float. This would be string-valued with allowed values:

• always - always convert to float
• default - convert but allow context-specific override
• never - never convert to float

With this you could do:

with conf.set_temp("quantity_convert_int_to_float", "never"):
    q = [1, 2] * u.ct  # int!

Then QTable init would do something like if conf.quantity_convert_int_to_float != "default" and set the config temporarily to "never" when calling the Quantity constructor. That means the default behavior for adding an int Column to a QTable will change. After staring at the docs this seems like not such a huge impact.

With that default change, I think that no changes would be required to any of the I/O layers like ECSV or FITS, but I haven't thought it through entirely.

The change in this PR will require adding the new argument to QTable.read() which is not so nice.

Yes, I think giving the user broader choice of what Quantity does is reasonable. (Since it could potentially affect science, it may be better implemented as a ScienceState, though that also feels weird; anyway, ignore that for the moment, let's assume there is some context manager that can be used.)

For the present PR, I think you meant that in the "default" case, you allow conversion, but if it is "never" or "always", you just follow what the user asked (i.e., the logic should read a bit different from what you had, if conf.quantity_convert_int_to_float == "default", temporarily set to "never"). So, the default behaviour would become like what passing in dtype=None would do. I do think that is reasonable, especially if this way the user can get the old behaviour back.

Closed in favor of #20273