allow ; in get/setitem #983
Conversation
so that we can target multiple axes in a single string. eg. arr['a1;b1'] or arr['b1..b3;a2,a1']
There was a problem hiding this comment.
- I wonder if the tutorial section about selections shouldn't be updated ?
- I wonder if all the logic about keys should not be moved from the
group.pymodule to a separate new module (since, as I understand it, the functions present ingroup.pyare also used inaxis.pyandarray.py)?
| @@ -2811,6 +2811,12 @@ def _key_to_axis_indices_dict(self, key): | |||
| """ | |||
| from .array import Array | |||
|
|
|||
| if isinstance(key, str) and ';' in key: | |||
| # FIXME: it would be more logical to use _to_keys(key) instead but this breaks | |||
| # the "target an aggregate with its key" feature (test_getitem_on_group_agg). | |||
There was a problem hiding this comment.
Sorry but I don't understand what you mean by "target an aggregate with its key" ?
There was a problem hiding this comment.
The fact that the two last lines both work:
>>> arr = ndtest(4)
>>> arr
a a0 a1 a2 a3
0 1 2 3
>>> a = arr.a
>>> g1 = a['a1,a3'] >> 'g1'
>>> g2 = a['a0,a2']
>>> agg = arr.sum((g1, g2))
>>> agg
a g1 a0,a2
4 2
>>> agg[g1]
4
>>> agg[g2]
2This is an obscure feature of LArray which I thought users would want to use but was a pain to implement and "keep working over the years". It makes the code ugly in a few places and costs a bit of performance too... and AFAIK nobody uses it so we should scrap it I think.
There was a problem hiding this comment.
OK. In that case, add an issue for that (or we will rapidly completely forget about it). Thx.
| @@ -651,6 +651,14 @@ def test_getitem_guess_axis(array): | |||
| res = arr['b[l1]'] | |||
| assert_array_equal(res, arr.data[:, 0]) | |||
|
|
|||
| # several axes in same string (guess axis) | |||
| res = arr['l0;l2'] | |||
There was a problem hiding this comment.
update the tutorial with a similar example?
| assert res == 1 | ||
|
|
||
| # several axes in same string (with axis information) | ||
| res = arr['a[l1];b[l2]'] |
Yes indeed. Thanks for keeping my lazyness in check 😄.
That would make sense but I don't know if that's easily feasible (cyclic imports etc.). Needs to check. |
Maybe adding an |
No description provided.