Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group aggregate has wrong (and different than getitem!) behavior for groups from other axes #1117

Open
gdementen opened this issue Oct 2, 2024 · 1 comment
Assignees
Milestone

Comments

@gdementen
Copy link
Contributor

>>> arr = ndtest("a=a0,a2,a1")
>>> arr
a  a0  a2  a1
    0   1   2
>>> g = arr.a.i[1]
>>> g
a.i[1]
>>> arr[g]
1
>>> arr.sum(g)
1

Now let us define another array with the same axis name, and reuse the g group:

>>> arr2 = ndtest("a=a0..a2")
>>> arr2
a  a0  a1  a2
    0   1   2

For getitem, the group is retargeted to the "local array" axis (i.e. arr2.a) by using the labels corresponding to the group on the original axis:

>>> arr2[g]
2

But for group aggregates, this is the position, irrespective of the label:

>>> arr2.sum(g)
1

This is because of the:

axis_key = axis_key.with_axis(real_axis)

line in AxisCollection._guess_axis which should probably use retarget_to instead.

@gdementen gdementen added this to the 0.35 milestone Oct 2, 2024
@gdementen gdementen changed the title Group aggregate has wrong (and different than getitem!) behavior for igroups from another axis Group aggregate has wrong (and different than getitem!) behavior for groups from other axes Oct 2, 2024
@gdementen
Copy link
Contributor Author

In fact, the problem caused by that line is present for LGroup too (but there only for slice groups):

>>> arr = ndtest(3)
>>> arr
a  a0  a1  a2
    0   1   2
>>> a2 = Axis('a=a0,a2,a1')
>>> arr.sum(arr.a['a0:a1']) == 1
>>> arr.sum(a2['a0:a1']) # should be 3
1

And in fact, I just noticed I had already found and fixed this bug in one of my local branches (slice_groups_incompatible_axis) almost two years ago. Pffff... 😭

I am not 100% sure we should have this retarget behavior in the first place. Both can be confusing. But I am 100% sure getitem and group aggregate should behave the same.

@gdementen gdementen self-assigned this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant