Gp/feat/aman arbitrary path #1057

iparask · 2024-12-04T18:05:01Z

This PR updates AxisManager __getitem__ and __setitem__ based on the slack discussion.

__setitem__ raises an ValueError when the value is not another AxisManager and uses wrap to add it to the main object. I selected wrap because it seemed more complete compared to merge.

mhasself

Looks good -- a couple of requests though:

get and __contains__ have not been changed. I think we should support the same treatment for keys provided there (but this is probably debatable).
The tests don't test very much right now... please expand.
Mention this behavior in the docs (see axisman.rst, add a few lines after paragraph of "Note the boresight entry is marked with")

Thanks!

skhrg

I think mattew covered everything I had in mind.

I think the answer to this is "not enough that we care" or "not enough to measure" but is there any noticeable slowdown from accessing children in this way?

Expanding the test case so show that the values that we get and set are correct I think is important.

iparask · 2024-12-04T21:00:09Z

get and __contains__ have not been changed. I think we should support the same treatment for keys provided there (but this is probably debatable).

Get is a combination of __contains__ and __getitem__ the way it is implemented. It's behavior change is a side effect of the rest of the changes.

I think the answer to this is "not enough that we care" or "not enough to measure" but is there any noticeable slowdown from accessing children in this way?

Yes, this is measurable but how many levels down do you want to go?

skhrg · 2024-12-04T21:04:16Z

Yes, this is measurable but how many levels down do you want to go?

I think most people will never go beyond triple nested so if that slowdown is in the noise then I think we are good. If it somehow matters this seems like a case where we could cache the mapping from the full path to where they point to alleviate things (but I really dont think it will end up being an issue).

mhasself · 2024-12-06T07:06:01Z

get and __contains__ have not been changed. I think we should support the same treatment for keys provided there (but this is probably debatable).

Get is a combination of __contains__ and __getitem__ the way it is implemented. It's behavior change is a side effect of the rest of the changes.

Having changed __getitem__, we need to think about whether to change .keys (no?) and __contains__ (yes?).

It is nice to be able to check if k in aman -- answering the question "will aman[k] raise a KeyError"?

iparask · 2024-12-09T17:15:20Z

It is nice to be able to check if k in aman -- answering the question "will aman[k] raise a KeyError"?

>>> from sotodlib import core
>>> import numpy as np
>>> dets = ["det0", "det1", "det2"]
>>> n, ofs = 1000, 0
>>> aman = core.AxisManager(
...     core.LabelAxis("dets", dets), core.OffsetAxis("samps", n, ofs)
... )
>>> child = core.AxisManager(
...     core.LabelAxis("dets", dets + ["det3"]),
...     core.OffsetAxis("samps", n, ofs - n // 2),
... )
>>> aman.wrap("child", child)
AxisManager(child*[dets,samps], dets:LabelAxis(3), samps:OffsetAxis(500))
>>> "child.samps" in aman
True
>>> "child.something" in aman
False
>>>

Is this what you mean? It is in the commit that addresses the comments. I'm trying to get them all in a single push.

mhasself · 2024-12-09T19:04:55Z

Is this what you mean? It is in the commit that addresses the comments. I'm trying to get them all in a single push.

Yes, that's what I mean. Thanks.

iparask · 2024-12-09T21:10:37Z

I am not sure if the test makes sense in terms of operations. Let me know if I need to update it in a way that reflects the correct usage of the AxisManager class.

mhasself

Thanks.

I think what's missing in the test is just a simple data field, such as a shape=(samps,) array, in child or child2. Check that accessing, and setting, that field using 'child.child2.target' works properly.

tests/test_core.py

iparask · 2024-12-12T17:02:29Z

I updated the tests and added a _fields tod to the test. I'm really confused as to why __setattr__ and __setitem__ have different functionalities. It seems like a bug.

I also timed them, using timeit and 1000 iterations and these are the results:

(simonsobs) iparask@simons1:~/git/sotodlib$ python tests/simple_example.py
Time to access aman['child.child2.tod'] from this branch: 0.0009413734078407288

(simonsobs) iparask@simons1:~/git/sotodlib$ python tests/simple_example.py
Time to access aman.child.child2.tod from sotodlib master branch: 0.0019882768392562866

I consider these to be the same time.

skhrg

Looks good to me, I like the test a lot.
I have one minor issue with the docs and one potential feature to add but overall I think this is good to go.

docs/axisman.rst

skhrg · 2024-12-16T16:05:04Z

tests/test_core.py

+        )
+        child2.wrap("tod", np.zeros((2,1000)))
+        aman.wrap("child", child)
+        aman["child"].wrap("child2", child2)


@mhasself do we want support in wrap for these paths as well? ie aman.wrap('child.child2', ...)?

I am ok with that but let's leave it out for now.

iparask · 2024-12-20T14:52:14Z

@mhasself can you take another look?

mhasself

Thanks -- a few more comments.

mhasself · 2024-12-26T06:25:42Z

tests/test_core.py

@@ -3,6 +3,7 @@
 import os
 import shutil

+from networkx import selfloop_edges


Unused import?

mhasself · 2024-12-26T06:27:37Z

tests/test_core.py

@@ -298,6 +299,56 @@ def test_300_restrict(self):
        self.assertNotEqual(aman.a1[0, 0, 0, 1], 0.)

    # wrap of AxisManager, merge.
+    def test_get_set(self):


The naming convention here is test_NNN_description. I propose you call this test_190_get_set and move it upwards, before the # Multi-dimensional restrictions. section.

mhasself · 2024-12-26T06:29:24Z

tests/test_core.py

+        )
+        child2.wrap("tod", np.zeros((2,1000)))
+        aman.wrap("child", child)
+        aman["child"].wrap("child2", child2)


I am ok with that but let's leave it out for now.

tests/test_core.py

mhasself · 2024-12-26T06:57:30Z

sotodlib/core/axisman.py

+        if isinstance(val, AxisManager) and isinstance(tmp_item, AxisManager):
+            raise ValueError("Cannot assign AxisManager to AxisManager. Please use wrap method.")
+
+        tmp_item.__setattr__(val_key, val)


This is a slight regression -- consider:

a = AxisManager() a.x = 1 a['y'] = 1

The previous behavior was that a.x = 1 would work, but a['y'] = 1 would raise a KeyError. Please restore that behavior.

mhasself · 2024-12-26T06:58:24Z

tests/test_core.py

+
+        self.assertIn("child.dets", aman)
+        self.assertIn("child.dets2", aman) # I am not sure why this is true
+        self.assertNotIn("child.child2.someentry", aman)


add test
self.assertNotIn("child.child2.someentry.someotherentry", aman)

mhasself · 2024-12-26T07:08:38Z

docs/axisman.rst

@@ -144,6 +144,26 @@ The output of the ``wrap`` cal should be::
 Note the boresight entry is marked with a ``*``, indicating that it's
 an AxisManager rather than a numpy array.

+Data access under an AxisManager is done based on field names. For example:


In your example, you construct a new aman. But in the flow of this documentation, an AxisManager already exists, which has been built up demonstrating the features and usage. The existing one has enough stuff to demonstrate all the features you need to demonstrate (e.g. dset.boresight.az vs. dset['boresight.az']). So please use that! (And that should flow into the "To slice this object..." section, which again refers to the dset variable.)

Also this line for some reason renders in bold, in the docs... maybe add a blank line after? Not sure how you got away without a double ::...

iparask added 2 commits December 3, 2024 10:06

wip: functionality added, missing tests

77407ad

updating setitem to accept only AxisManager

1cfd3b9

iparask requested review from mhasself and skhrg December 4, 2024 18:05

iparask added 2 commits December 4, 2024 13:22

wip: somewhat reversing setitem

a7f1faf

fixed incoming bug and added test

c414e67

iparask marked this pull request as ready for review December 4, 2024 19:32

mhasself requested changes Dec 4, 2024

View reviewed changes

skhrg reviewed Dec 4, 2024

View reviewed changes

iparask added 2 commits December 9, 2024 16:07

Addressing PR comments

eed780a

adding missing test

29500b1

mhasself requested changes Dec 9, 2024

View reviewed changes

tests/test_core.py Show resolved Hide resolved

tests/test_core.py Outdated Show resolved Hide resolved

tests updated

dcc805c

iparask requested review from mhasself and skhrg December 12, 2024 17:02

skhrg requested changes Dec 16, 2024

View reviewed changes

fixing docs to show advanced usage

d316c51

mhasself requested changes Dec 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gp/feat/aman arbitrary path #1057

Gp/feat/aman arbitrary path #1057

iparask commented Dec 4, 2024

mhasself left a comment

skhrg left a comment

iparask commented Dec 4, 2024

skhrg commented Dec 4, 2024 •

edited

Loading

mhasself commented Dec 6, 2024

iparask commented Dec 9, 2024

mhasself commented Dec 9, 2024

iparask commented Dec 9, 2024

mhasself left a comment

iparask commented Dec 12, 2024

skhrg left a comment

skhrg Dec 16, 2024

mhasself Dec 26, 2024

iparask commented Dec 20, 2024

mhasself left a comment

mhasself Dec 26, 2024

mhasself Dec 26, 2024

mhasself Dec 26, 2024

mhasself Dec 26, 2024

mhasself Dec 26, 2024

mhasself Dec 26, 2024

Gp/feat/aman arbitrary path #1057

Are you sure you want to change the base?

Gp/feat/aman arbitrary path #1057

Conversation

iparask commented Dec 4, 2024

mhasself left a comment

Choose a reason for hiding this comment

skhrg left a comment

Choose a reason for hiding this comment

iparask commented Dec 4, 2024

skhrg commented Dec 4, 2024 • edited Loading

mhasself commented Dec 6, 2024

iparask commented Dec 9, 2024

mhasself commented Dec 9, 2024

iparask commented Dec 9, 2024

mhasself left a comment

Choose a reason for hiding this comment

iparask commented Dec 12, 2024

skhrg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iparask commented Dec 20, 2024

mhasself left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skhrg commented Dec 4, 2024 •

edited

Loading