Repository: astropy/astropy

Issue:
Deepcopy of table with object column returns shallow copy
<!-- This comments are hidden when you submit the issue,
so you do not need to remove them! -->

<!-- Please be sure to check out our contributing guidelines,
https://github.com/astropy/astropy/blob/main/CONTRIBUTING.md .
Please be sure to check out our code of conduct,
https://github.com/astropy/astropy/blob/main/CODE_OF_CONDUCT.md . -->

<!-- Please have a search on our GitHub repository to see if a similar
issue has already been posted.
If a similar issue is closed, have a quick look to see if you are satisfied
by the resolution.
If not please go ahead and open an issue! -->

<!-- Please check that the development version still produces the same bug.
You can install development version with
pip install git+https://github.com/astropy/astropy
command. -->

### Description
With an astropy `Table` (or `QTable`) containing a column of `dtype=object`, a deepcopy returns a shallow copy of that column (the other columns are OK).

### Expected behavior
Deep copies of all columns

### Actual behavior
Deep copies of columns with `dtype=int`, `float`, etc, but with `object` just returns shallow copy.

### Steps to Reproduce

```python
import copy
from astropy.table import Table
from astropy import units as u
import numpy as np
import astropy


class X:
    def __init__(self, value):
        self.value = value

    def __repr__(self):
        return str(self.value)


a = np.array([1, 4, 5], dtype=np.int32)
b = [2.0, 5.0, 8.5]
c = ["x", "y", "z"]
d = [10, 20, 30] * u.m / u.s
e = [X(1), X(2), X(3)]

t = Table(
    [a, b, c, d, e],
    names=("a", "b", "c", "d", "e"),
    meta={"name": "first table"},
)
t_copy = copy.deepcopy(t)

for cname in t.colnames:
    print(
        f"column '{cname}' in Table: type={t[cname].dtype}, is identical={t[0][cname] is t_copy[0][cname]}"
    )
print(f"For astropy version {astropy.__version__}")
```

For me this returns:
```
column 'a' in Table: type=int32, is identical=False
column 'b' in Table: type=float64, is identical=False
column 'c' in Table: type=<U1, is identical=False
column 'd' in Table: type=float64, is identical=False
column 'e' in Table: type=object, is identical=True
For astropy version 5.1
```

If you add in an explicit deepcopy of those objects:
```python
for i in range(len(t)):
    t_copy[i]["e"] = copy.deepcopy(t[i]["e"])
```
then it works as expected

### System Details
```
>>> import platform; print(platform.platform())
macOS-10.16-x86_64-i386-64bit
>>> import sys; print("Python", sys.version)
Python 3.9.7 (default, Sep 16 2021, 08:50:36) 
[Clang 10.0.0 ]
>>> import numpy; print("Numpy", numpy.__version__)
Numpy 1.21.2
>>> import erfa; print("pyerfa", erfa.__version__)
pyerfa 2.0.0
>>> import astropy; print("astropy", astropy.__version__)
astropy 5.1
>>> import scipy; print("Scipy", scipy.__version__)
Scipy 1.7.1
>>> import matplotlib; print("Matplotlib", matplotlib.__version__)
Matplotlib 3.4.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?
  - [ ] Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  - [ ] 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.
github-actions[bot]: 👋 Thank you for your draft pull request! Do you know that you can use `[ci skip]` or `[skip ci]` in your commit messages to skip running continuous integration tests until you are ready?
neutrinoceros: @pllim Do you see a reason not to backport this to 6.0.x ?
neutrinoceros: Thanks for your input, I didn't realize that `Column` indeed already behaves like its parent class so the fix needs to happen some place else. I played around with the patch you suggested in the issue but it breaks how metadata and indices gets passed to copies. I've readjusted the test cases to the now clarified spec (locally), and I'll push when I manage to pass all tests locally.
taldcroft: Sorry, I entirely missed this. I'll have a look but I'm stepping away for the afternoon now. One quick thing is with `self.copy(False)` I'm not sure about `.meta` getting a deep copy.
taldcroft: To answer my question, despite the documentation `meta` is not actually deepcopied unless `copy=True`.
```
>>> meta = {1: object()}
>>> t1 = Table([[1]], meta=meta)
>>> t2_nc = Table(t1, copy=False)
>>> t2_c = Table(t1, copy=True)
>>> t1.meta[1] is t2_nc.meta[1]
True
>>> t1.meta[1] is t2_c.meta[1]
False
```
This stems from https://github.com/astropy/astropy/pull/8404. It would appear in that PR that we entirely failed to update related docstrings about the behavior of `.meta` and `copy`. Uff.
taldcroft: We should take this opportunity to clean up the docs. In the `Table` and `QTable` docstrings update the `copy` kwarg to: 
```
    copy : bool, optional
        Copy the input column data and make a deep copy of the input meta.
        Default is True.
```
Update the `copy()` docstring to:
```
        copy_data : bool
            If `True` (the default), copy the underlying data array and make
            a deep copy of the ``meta`` attribute. Otherwise, use the same 
            data array and make a shallow (key-only) copy of ``meta``.
```

neutrinoceros: Thanks @taldcroft, I've added your suggestions to the batch !
Btw, I've kept the branch history as additive as possible on this one to simplifying reviews, so please squash-merge !
neutrinoceros: (for reference, the conflict was with https://github.com/astropy/astropy/pull/16002)
neutrinoceros: rebased !

Patch:
diff --git a/astropy/table/table.py b/astropy/table/table.py
index e0c217318f..be72ab1161 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -608,8 +608,7 @@ class Table:
     meta : dict, optional
         Metadata associated with the table.
     copy : bool, optional
-        Copy the input data. If the input is a Table the ``meta`` is always
-        copied regardless of the ``copy`` parameter.
+        Copy the input column data and make a deep copy of the input meta.
         Default is True.
     rows : numpy ndarray, list of list, optional
         Row-oriented data for table instead of ``data`` argument.
@@ -3777,9 +3776,9 @@ class Table:
         Parameters
         ----------
         copy_data : bool
-            If `True` (the default), copy the underlying data array.
-            Otherwise, use the same data array. The ``meta`` is always
-            deepcopied regardless of the value for ``copy_data``.
+            If `True` (the default), copy the underlying data array and make
+            a deep copy of the ``meta`` attribute. Otherwise, use the same
+            data array and make a shallow (key-only) copy of ``meta``.
         """
         out = self.__class__(self, copy=copy_data)
 
@@ -3791,7 +3790,11 @@ class Table:
         return out
 
     def __deepcopy__(self, memo=None):
-        return self.copy(True)
+        out = self.copy(False)
+        for name in out.colnames:
+            out.columns.__setitem__(name, deepcopy(self[name]), validated=True)
+        out.meta = deepcopy(self.meta)
+        return out
 
     def __copy__(self):
         return self.copy(False)
diff --git a/docs/changes/table/15871.bugfix.rst b/docs/changes/table/15871.bugfix.rst
new file mode 100644
index 0000000000..f91a5c5920
--- /dev/null
+++ b/docs/changes/table/15871.bugfix.rst
@@ -0,0 +1,1 @@
+Fix a bug where columns with ``dtype=object`` wouldn't be properly deep-copied using ``copy.deepcopy``.


Test patch:
diff --git a/astropy/table/tests/test_column.py b/astropy/table/tests/test_column.py
index 2ced5a5cfe..aa76bd114b 100644
--- a/astropy/table/tests/test_column.py
+++ b/astropy/table/tests/test_column.py
@@ -1,5 +1,6 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
 
+import copy
 import operator
 import warnings
 
@@ -464,6 +465,24 @@ class TestColumn:
             t["a"].mask = [True, False]
 
 
+@pytest.mark.parametrize(
+    "data",
+    [np.array([object()]), [object()]],
+)
+def test_deepcopy_object_column(data):
+    # see https://github.com/astropy/astropy/issues/13435
+    c1 = table.Column(data, meta={"test": object()})
+    c2 = copy.deepcopy(c1)
+    assert c2 is not c1
+    assert c2[0] is not c1[0]
+    assert c2.meta["test"] is not c1.meta["test"]
+
+    c3 = table.Column(c1, copy=True)
+    assert c3 is not c1
+    assert c3[0] is c1[0]
+    assert c3.meta["test"] is not c1.meta["test"]
+
+
 class TestAttrEqual:
     """Bunch of tests originally from ATpy that test the attrs_equal method."""
 
diff --git a/astropy/table/tests/test_table.py b/astropy/table/tests/test_table.py
index a15f8e3763..3b4bd9896b 100644
--- a/astropy/table/tests/test_table.py
+++ b/astropy/table/tests/test_table.py
@@ -2720,6 +2720,27 @@ def test_table_meta_copy_with_meta_arg():
     assert t.meta[1] is not meta[1]
 
 
+@pytest.mark.parametrize(
+    "data",
+    [np.array([object()]), [object()]],
+)
+def test_deepcopy_object_column(data):
+    # see https://github.com/astropy/astropy/issues/13435
+    t1 = Table({"a": data}, meta={"test": object()})
+    t2 = copy.deepcopy(t1)
+    c1 = t1["a"]
+    c2 = t2["a"]
+    assert c2 is not c1
+    assert c2[0] is not c1[0]
+    assert t2.meta["test"] is not t1.meta["test"]
+
+    t3 = Table(t1, copy=True)
+    c3 = t3["a"]
+    assert c3 is not c1
+    assert c3[0] is c1[0]
+    assert t3.meta["test"] is not t1.meta["test"]
+
+
 def test_replace_column_qtable():
     """Replace existing Quantity column with a new column in a QTable"""
     a = [1, 2, 3] * u.m