Repository: astropy/astropy

Issue:
QTable, mask=True and FalseArray
### Description

The behavior of the `masked=True` is counter intuitive. If used, it assign an `astropy.columns.FalseArray` to each column, which in principle can not be set to `True` but  an inplace change could set the value to True silently, although it is not used later.

The inplace changes is fundamental when using very long table as it avoids memory allocation.


### Expected behavior

The inplace operation should fail as per the `FalseArray` definition or, I believe a better solution, use a proper numpy array for the mask in `QTable`, which is actually the case for `Table`

```python 
from astropy.table import Table
tab = Table([[1, 1]], names=['test'], masked=True)
#tab['test'].mask is array([False, False])
```

### How to Reproduce


```python 
import astropy.units as u
from astropy.table import QTable
tab = QTable([[1, 1] * u.mJy], names=['test'], masked=True)
# tab['test'].mask is FalseArray([False, False])
# tab['test'].mask[0] = True # Will raise a `ValueError` as expected from `FalseArray` 
tab['test'].mask |= [True, True] # Will work without a warning
# tab['test'] is now  FalseArray([ True,  True])
```

### Versions

Linux-6.7.9-amd64-x86_64-with-glibc2.38
Python 3.11.9 (main, Apr 10 2024, 13:16:36) [GCC 13.2.0]
astropy 6.0.1
Numpy 1.26.4
pyerfa 2.0.1.4
Scipy 1.11.4
Matplotlib 3.6.3


Discussion:
github-actions[bot]: 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](https://docs.astropy.org/en/latest/development/codeguide.html)?
  - [ ] Are tests added/updated as required? If so, do they follow the [Astropy testing guidelines](https://docs.astropy.org/en/latest/development/testguide.html)?
  - [ ] Are docs added/updated as required? If so, do they follow the [Astropy documentation guidelines](https://docs.astropy.org/en/latest/development/docguide.html#astropy-documentation-rules-and-guidelines)?
  - [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for [rebase](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#rebase-if-necessary) and [squash](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#squash-if-necessary).
  - [ ] 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](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit).
  - [ ] 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.
mhvk: @taldcroft - this the follow-up PR that ensures `MaskedQuantity` is used more consistently in `QTable`, as needed. I think it is more or less OK, but must admit I've gotten a bit lost in the many places in `table.py` where conversions to other classes are done - `_convert_data_to_col`, `_convert_col_for_table`, and `_get_col_cls_for_table`. It may be possible to refactor that somewhat, but out of scope here... I did try to check that all the additions I made are actually needed (and tested).

Note that the PR depends on commits that are not backported, so we cannot backport this either. Probably for the better, since arguably the API changes a little.
taldcroft: @mhvk - this looks great. The only comment I have is that this should probably be listed as an API change. Fundamentally what is changing is that `Quantity` with `FalseArray` mask is being replaced by `MaskedQuantity`.

```
Always use ``MaskedQuantity`` in ``QTable`` to represent masked `Quantity`
data or when the the ``QTable`` is created with `masked=True`.  Previously 
the default was to use a normal ``Quantity`` with a ``mask`` attribute of type
``FalseArray`` as a stub to allow a minimal level of compatibility for certain
operations. This update brings more consistent behavior and fixes functions 
like reading of table data from a list of dict that includes quantities with missing 
entries, and aggregation of ``MaskedQuantity`` in table groups.
```

Maybe the bits that were specifically broken could be put into a bugfix change. Note that the actual *bug* in 16495 is that `FalseArray` is weak and `q.mask |= [...]` silently works. This is not being fixed but circumvented.
taldcroft: We might also consider whether this is worth a What's New.
mhvk: @taldcroft - thanks for the review. I think you're right that as an API change it makes more sense. I also added a short what's new entry.

Patch:
diff --git a/astropy/table/table.py b/astropy/table/table.py
index c91f692de9..22739c6e5c 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -1234,10 +1234,11 @@ class Table:
             for name, indexes in missing_indexes.items():
                 col = self[name]
                 # Ensure that any Column subclasses with MISSING values can support
-                # setting masked values. As of astropy 4.0 the test condition below is
-                # always True since _init_from_dict cannot result in mixin columns.
+                # setting masked values.
                 if isinstance(col, Column) and not isinstance(col, MaskedColumn):
                     self[name] = self.MaskedColumn(col, copy=False)
+                elif isinstance(col, Quantity) and not isinstance(col, Masked):
+                    self[name] = Masked(col)
 
                 # Finally do the masking in a mixin-safe way.
                 self[name][indexes] = np.ma.masked
@@ -1454,6 +1455,8 @@ class Table:
         if self.masked:
             if isinstance(col, Column) and not isinstance(col, self.MaskedColumn):
                 col_cls = self.MaskedColumn
+            elif isinstance(col, Quantity) and not isinstance(col, Masked):
+                col_cls = Masked(col_cls)
         else:
             if isinstance(col, MaskedColumn):
                 if not isinstance(col, self.MaskedColumn):
@@ -4363,7 +4366,11 @@ class QTable(Table):
         return has_info_class(col, MixinInfo)
 
     def _convert_col_for_table(self, col):
-        if isinstance(col, Column) and getattr(col, "unit", None) is not None:
+        if isinstance(col, Quantity):
+            if self.masked and not isinstance(col, Masked):
+                col = Masked(col)
+
+        elif isinstance(col, Column) and getattr(col, "unit", None) is not None:
             # We need to turn the column into a quantity; use subok=True to allow
             # Quantity subclasses identified in the unit (such as u.mag()).
             q_cls = Masked(Quantity) if isinstance(col, MaskedColumn) else Quantity
@@ -4384,3 +4391,13 @@ class QTable(Table):
             col = super()._convert_col_for_table(col)
 
         return col
+
+    def _convert_data_to_col(
+        self, data, copy=True, default_name=None, dtype=None, name=None
+    ):
+        if self.masked and isinstance(data, Quantity):
+            data = Masked(data)
+
+        return super()._convert_data_to_col(
+            data, copy=copy, default_name=default_name, dtype=dtype, name=name
+        )
diff --git a/docs/changes/table/16500.api.rst b/docs/changes/table/16500.api.rst
new file mode 100644
index 0000000000..0e3500730a
--- /dev/null
+++ b/docs/changes/table/16500.api.rst
@@ -0,0 +1,7 @@
+Always use ``MaskedQuantity`` in ``QTable`` to represent masked ``Quantity``
+data or when the ``QTable`` is created with ``masked=True``.  Previously the
+default was to use a normal ``Quantity`` with a ``mask`` attribute of type
+``FalseArray`` as a stub to allow a minimal level of compatibility for certain
+operations. This update brings more consistent behavior and fixes functions
+like reading of table data from a list of dict that includes quantities with
+missing entries, and aggregation of ``MaskedQuantity`` in table groups.
diff --git a/docs/whatsnew/7.0.rst b/docs/whatsnew/7.0.rst
index c2c396a08c..ca388cb92a 100644
--- a/docs/whatsnew/7.0.rst
+++ b/docs/whatsnew/7.0.rst
@@ -12,6 +12,7 @@ the 6.1 release.
 
 In particular, this release includes:
 
+* :ref:`whatsnew-7.0-table-masked-quantity`
 * :ref:`whatsnew_7_0_quantity_to_string_formatter`
 * :ref:`whatsnew_7_0_ecsv_meta_default_dict`
 * :ref:`whatsnew_7_0_contributor_doc_improvement`
@@ -24,6 +25,20 @@ By the numbers:
 * X pull requests have been merged since v6.1
 * X distinct people have contributed code
 
+.. _whatsnew-7.0-table-masked-quantity:
+
+Full ``MaskedQuantity`` Support in ``QTable``
+=============================================
+
+Masked quantities were already used in many table functions, like reading from
+files, and are now fully supported throughout, i.e., ``MaskedQuantity`` are
+now always used in ``QTable`` to represent masked quantities (or when the
+``QTable`` is created with ``masked=True``). This removes the last vestiges of
+a work-around where a normal ``Quantity`` was used with a stub of a mask, and
+fixes functions like reading of table data from a list of dict that includes
+quantities with missing entries, and aggregation of ``MaskedQuantity`` in
+table groups.
+
 .. _whatsnew_7_0_quantity_to_string_formatter:
 
 ``Quantity.to_string`` supports ``formatter`` for formatting


Test patch:
diff --git a/astropy/table/tests/test_groups.py b/astropy/table/tests/test_groups.py
index c9318561bd..64014383df 100644
--- a/astropy/table/tests/test_groups.py
+++ b/astropy/table/tests/test_groups.py
@@ -469,6 +469,7 @@ def test_table_aggregate(T1):
     t1m = QTable(T1, masked=True)
     t1m["c"].mask[4:6] = True
     t1m["d"].mask[4:6] = True
+    t1m["q"].mask[4:6] = True
     tg = t1m.group_by("a")
 
     if PYTEST_LT_8_0:
@@ -483,8 +484,8 @@ def test_table_aggregate(T1):
         " a   c    d    q  ",
         "               m  ",
         "--- ---- ---- ----",
-        "  0   --   --  4.0",
-        "  1  3.0 13.0 18.0",
+        "  0   --   --  ———",
+        "  1  3.0 13.0  ———",
         "  2 22.0  6.0  6.0",
     ]
 
diff --git a/astropy/table/tests/test_init_table.py b/astropy/table/tests/test_init_table.py
index ec66f181d5..413fdf820a 100644
--- a/astropy/table/tests/test_init_table.py
+++ b/astropy/table/tests/test_init_table.py
@@ -5,9 +5,11 @@ from collections.abc import Mapping
 
 import numpy as np
 import pytest
+from numpy.testing import assert_array_equal
 
 import astropy.units as u
 from astropy.table import Column, MaskedColumn, QTable, Table, TableColumns
+from astropy.utils.masked import Masked
 
 
 class DictLike(Mapping):
@@ -223,6 +225,27 @@ class TestInitFromListOfDicts(BaseInitFromListLike):
             assert type(t["c"]) is MaskedColumn
 
 
+def test_qtable_uses_masked_quantity_as_needed():
+    data = [{"a": 1 * u.m, "b": 1}, {"a": 2 * u.Mm, "b": 2}]
+    data_ragged = [{"a": 1 * u.m, "b": 1}, {"a": 2 * u.Mm}, {"b": 3}]
+    t = QTable(data)
+    assert t.colnames == ["a", "b"]
+    assert isinstance(t["a"], u.Quantity)
+    assert isinstance(t["b"], Column)
+    assert t["a"].unit == u.m
+    assert_array_equal(t["a"], [1, 2000000] * u.m)
+    assert not t.masked
+
+    t2 = QTable(data_ragged)
+    assert t2.colnames == ["a", "b"]
+    assert isinstance(t2["a"], Masked(u.Quantity))
+    assert isinstance(t2["b"], MaskedColumn)
+    assert t2["a"].unit == u.m
+    assert np.all(t2["a"] == [1, 2000000, 0] * u.m)
+    assert_array_equal(t2["a"].mask, [False, False, True])
+    assert_array_equal(t2["b"].mask, [False, True, False])
+
+
 class TestInitFromListOfMapping(TestInitFromListOfDicts):
     """Test that init from a Mapping that is not a dict subclass works"""
 
diff --git a/astropy/table/tests/test_masked.py b/astropy/table/tests/test_masked.py
index 0323b9de4d..de1aa08202 100644
--- a/astropy/table/tests/test_masked.py
+++ b/astropy/table/tests/test_masked.py
@@ -4,6 +4,7 @@
 import numpy as np
 import numpy.ma as ma
 import pytest
+from numpy.testing import assert_array_equal
 
 import astropy.units as u
 from astropy.table import Column, MaskedColumn, QTable, Table
@@ -595,6 +596,16 @@ def test_masked_column_with_unit_in_qtable():
     t["c"] = MaskedColumn([1, 2], unit=u.m, mask=[True, False])
     assert isinstance(t["c"], MaskedQuantity)
     assert np.all(t["c"].mask == [True, False])
+    # Regular Column is still converted to regular Quantity
+    t["d"] = Column([1, 2], unit=u.cm)
+    assert not isinstance(t["d"], MaskedQuantity)
+    assert isinstance(t["d"], u.Quantity)
+    # But not if the table is masked.
+    t2 = QTable(t, masked=True)
+    assert isinstance(t2["d"], MaskedQuantity)
+    t2["e"] = Column([1, 2], unit=u.cm)
+    assert isinstance(t2["e"], MaskedQuantity)
+    assert not np.any(t2["e"].mask)
 
 
 def test_masked_quantity_in_table():
@@ -608,6 +619,11 @@ def test_masked_quantity_in_table():
     assert isinstance(t["c"], MaskedColumn)
     assert np.all(t["c"].mask == [True, False])
 
+    t2 = Table(t, masked=True)
+    t2["d"] = u.Quantity([1, 2], unit=u.cm)
+    assert isinstance(t2["d"], MaskedColumn)
+    assert not np.any(t2["d"].mask)
+
 
 def test_masked_column_data_attribute_is_plain_masked_array():
     c = MaskedColumn([1, 2], mask=[False, True])
@@ -658,3 +674,15 @@ def test_set_masked_bytes_column():
     mc = MaskedColumn([b"a", b"b", b"c"], mask=mask)
     mc[:] = mc
     assert (mc.mask == mask).all()
+
+
+def test_qtable_masked_true_basics():
+    # Explicit regression test for gh-16495.
+    tab = QTable([[1, 1] * u.mJy], names=["test"], masked=True)
+    assert isinstance(tab["test"], Masked)
+    assert isinstance(tab["test"], u.Quantity)
+    assert not np.any(tab["test"].mask)
+    tab["test"].mask[0] = True
+    assert_array_equal(tab["test"].mask, [True, False])
+    tab["test"].mask |= [True, True]
+    assert_array_equal(tab["test"].mask, [True, True])