Fix view behavior for AwkwardArrays #1070

grst · 2023-07-24T18:31:16Z

Closes Future changes to Awkward Array behavior class resolution #1035
Follow up of Fix for awkward 2.3 #1040
Release note added (or unnecessary)

grst · 2023-07-24T18:35:20Z

I think this does the trick

codecov · 2023-07-24T18:42:02Z

Codecov Report

Merging #1070 (fdeff90) into main (2c8759d) will decrease coverage by 2.20%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1070      +/-   ##
==========================================
- Coverage   84.75%   82.56%   -2.20%     
==========================================
  Files          36       36              
  Lines        5149     5132      -17     
==========================================
- Hits         4364     4237     -127     
- Misses        785      895     +110

Flag	Coverage Δ
gpu-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
anndata/_io/specs/methods.py	`87.40% <ø> (-0.31%)`	⬇️
anndata/_core/aligned_mapping.py	`93.99% <100.00%> (+0.32%)`	⬆️
anndata/_core/views.py	`82.58% <100.00%> (-7.18%)`	⬇️

... and 5 files with indirect coverage changes

📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse).

flying-sheep

Shouldn’t this have some tests?

anndata/_core/views.py

grst · 2023-07-25T16:50:59Z

Shouldn’t this have some tests?

This is already covered by existing tests in test_views and test_awkward. The current version is not broken, but will break at some point in the future.

Co-authored-by: Angus Hollands <goosey15@gmail.com>

agoose77 · 2023-07-25T17:56:25Z

anndata/_core/views.py

-            array = ak.with_parameter(self, _PARAM_NAME, None)
-            array = ak.with_parameter(array, "__list__", None)
+            array = ak.with_parameter(array, _PARAM_NAME, None)
+            array = ak.with_parameter(array, "__record__", None)


This should use with_name, so that we can traverse into the layout to find the record-node

Suggested change

array = ak.with_parameter(array, "__record__", None)

array = ak.with_name(array, None)

I imagine that after our dicsussion @grst could not get that working (see #1040 (comment))

Is there a reason traversing is better than what we have (assuming @grst could not get the with_name working)? We only ever deal with this at a "top level."

agoose77 · 2023-07-25T17:59:17Z

@grst I get the sense that it might be helpful to have a meeting to discuss this. Would you be able to find time for a zoom at some point?

grst · 2023-07-27T14:58:58Z

Agreed, I sent an email with a poll to find a timeslot.

for more information, see https://pre-commit.ci

grst · 2023-09-01T07:54:57Z

@ivirshup, @ilan-gold, @agoose77, ready for re-review. Feels good to get rid of all that code 💥

Currently the tests fail for uns, but that seems to be a more general problem, see #1118.

agoose77 · 2023-09-01T20:18:16Z

@grst it is possible to modify the original array if you don't perform any row-based slices, i.e array["x"] = x, then this would modify array.

I can't recall to what extent we touched on this on the call. If the process of pulling out the Awkward Array always returns a shallow copy, this wouldn't be a problem.

grst · 2023-09-04T06:57:37Z

Hi @agoose77,

returning the shallow copy (or slice) works as expected. The remaining problem is that there's no handler implemented for the uns slot while subsetting AnnData -- regardless of whether it's an awkward array or a numpy array. I don't think we want to always return a shallow copy on __getitem__ -- I think there are legitimate use-cases where one wants to modify records.

ivirshup · 2023-09-05T18:38:33Z

The remaining problem is that there's no handler implemented for the uns slot while subsetting AnnData

Is that blocking for this PR, or tangential? I'd like to make a release candidate by the end of the week, so would like to merge soon if possible.

grst · 2023-09-06T06:38:04Z

If you don't mind, I can mark the respective tests as xfail. I think it's ok as

the behavior is not a regression of this PR, it was like that before
the problem is tracked in Modifying uns on a view changes the parent object #1118 and
I don't think it affects many users

ivirshup · 2023-09-07T11:26:17Z

I can mark the respective tests as xfail

Which tests are these?

grst · 2023-09-07T11:30:35Z

@ivirshup, these:
https://github.com/scverse/anndata/pull/1070/files#diff-1534693aa7219116bb36568d007d5f2116414d0c86148cfa15d859b861abafc7R143-R166

ivirshup · 2023-09-07T13:24:25Z

So just to clarify what this PR does. For this block:

import anndata as ad, awkward as ak, numpy as np

a = ad.AnnData(
    np.ones((3, 3)),
    obsm={"awk": ak.Array([{"a": 1}, {"a": 2}, {"a": 3}])}
)
v = a[:2]
v.obsm["awk"]["b"] = [5, 6]

display(v)
display(v.obsm["awk"])

On main:

<ipython-input-10-0f89ad3994c2>:8: ImplicitModificationWarning: Trying to modify attribute `.obsm` of view, initializing view as actual.
  v.obsm["awk"]["b"] = [5, 6]
AnnData object with n_obs × n_vars = 2 × 3
    obsm: 'awk'
<Array [{a: 1, b: 5}, {a: 2, b: 6}] type='2 * {a: int64, b: int64}'>

On this PR:

View of AnnData object with n_obs × n_vars = 2 × 3
    obsm: 'awk'
<Array [{a: 1}, {a: 2}] type='2 * {a: int64}'>

This is the desired behavior?

Is uns failed for other view tests? I don't know that I really see the need to specially include, then xfail these cases.

The test didn't catch that when modifying an awkward array in a AnnDataView, the changes did not persist within that view. The issue was that I had been retreiving the awkward array from the view once, and then did tests on it. Instead, to properly test this, I need to retreive the Awkward array from the AnnData view every time. It turns out doing so gives me a fresh copy of the origina awkward array and modifications only affect that copy - not the original data.

I solved the issue described in the previous commit by adding logic to `AlignedViewMixin` that can copy objects of a certain type (for now only awkward arrays) upon view creation.

grst · 2023-09-08T07:03:01Z

Thanks for catching this! I thought my tests cover this, but it turns out I retrieved the awkward array from the view only once and did all tests on that copy. Instead, retreiving v.obsm["awk"] multiple times always returns a fresh copy of the sliced awkward array which makes it impossible to modify it.

To solve this, it is necessary to perform a shallow copy of the awkward array on view creation (which is a cheap operation). I added this behavior here:
https://github.com/scverse/anndata/pull/1070/files#diff-af8b8ed3c983f1ed77b81030eb9d80df8136e0640324f180354e6e0d3b49e7e7R139-R159

Maybe this could also be useful for pandas data frames with copy-on-write behavior in the future?

ivirshup · 2023-09-08T16:28:13Z

I think caching the "view"s could be a good idea. I may need to think for a bit on how this effects my current thoughts on how to fix the garbage collection problem:

Fix garbage collection #1119

Currently, this won't work with the solution proposed there, since the AlignedMappings are created on the fly to prevent circular references from being formed.

Use __record__ instead of __list__ to store behavior

b23eda3

grst force-pushed the ak-array-views branch from 830a1bc to b23eda3 Compare July 24, 2023 18:33

grst marked this pull request as ready for review July 24, 2023 18:33

grst requested review from ivirshup and ilan-gold July 24, 2023 18:34

Fix views of views

b640617

flying-sheep requested changes Jul 25, 2023

View reviewed changes

agoose77 reviewed Jul 25, 2023

View reviewed changes

anndata/_core/views.py Outdated Show resolved Hide resolved

Update anndata/_core/views.py

843833f

Co-authored-by: Angus Hollands <goosey15@gmail.com>

agoose77 reviewed Jul 25, 2023

View reviewed changes

grst added 2 commits August 30, 2023 17:30

Remove all custom code for views

8aa09cc

WIP add test cases

86c5ff8

grst changed the title ~~AwkwardArrayViews: Use __record__ instead of __list__ to store behavior~~ Fix view behavior for AwkwardArrays Sep 1, 2023

grst and others added 2 commits September 1, 2023 09:41

Merge remote-tracking branch 'origin/main' into ak-array-views

a01b80e

[pre-commit.ci] auto fixes from pre-commit.com hooks

dfe7008

for more information, see https://pre-commit.ci

grst mentioned this pull request Sep 1, 2023

Modifying uns on a view changes the parent object #1118

Open

3 tasks

grst added 2 commits September 1, 2023 09:52

More test cases

8b14e57

Add comment about view behavior

4870553

Merge branch 'main' into ak-array-views

257c063

ivirshup added this to the 0.10.0 milestone Sep 7, 2023

ivirshup removed this from the 0.10.0 milestone Sep 7, 2023

grst added 2 commits September 8, 2023 08:51

Fix that awkward arrays in an awyward array view could not be modified.

fdeff90

I solved the issue described in the previous commit by adding logic to `AlignedViewMixin` that can copy objects of a certain type (for now only awkward arrays) upon view creation.

grst mentioned this pull request Oct 23, 2023

Future changes to Awkward Array behavior class resolution #1035

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix view behavior for AwkwardArrays #1070

Fix view behavior for AwkwardArrays #1070

grst commented Jul 24, 2023 •

edited by flying-sheep

Loading

grst commented Jul 24, 2023

codecov bot commented Jul 24, 2023 •

edited

Loading

flying-sheep left a comment

grst commented Jul 25, 2023

agoose77 Jul 25, 2023

ilan-gold Jul 26, 2023

agoose77 commented Jul 25, 2023

grst commented Jul 27, 2023

grst commented Sep 1, 2023

agoose77 commented Sep 1, 2023

grst commented Sep 4, 2023

ivirshup commented Sep 5, 2023

grst commented Sep 6, 2023

ivirshup commented Sep 7, 2023

grst commented Sep 7, 2023

ivirshup commented Sep 7, 2023

grst commented Sep 8, 2023

ivirshup commented Sep 8, 2023

	array = ak.with_parameter(array, "__record__", None)
	array = ak.with_name(array, None)

Fix view behavior for AwkwardArrays #1070

Are you sure you want to change the base?

Fix view behavior for AwkwardArrays #1070

Conversation

grst commented Jul 24, 2023 • edited by flying-sheep Loading

grst commented Jul 24, 2023

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

flying-sheep left a comment

Choose a reason for hiding this comment

grst commented Jul 25, 2023

agoose77 Jul 25, 2023

Choose a reason for hiding this comment

ilan-gold Jul 26, 2023

Choose a reason for hiding this comment

agoose77 commented Jul 25, 2023

grst commented Jul 27, 2023

grst commented Sep 1, 2023

agoose77 commented Sep 1, 2023

grst commented Sep 4, 2023

ivirshup commented Sep 5, 2023

grst commented Sep 6, 2023

ivirshup commented Sep 7, 2023

grst commented Sep 7, 2023

ivirshup commented Sep 7, 2023

grst commented Sep 8, 2023

ivirshup commented Sep 8, 2023

grst commented Jul 24, 2023 •

edited by flying-sheep

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading