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

Functions for detector splits #821

Merged
merged 5 commits into from
May 1, 2024
Merged

Functions for detector splits #821

merged 5 commits into from
May 1, 2024

Conversation

chervias
Copy link
Contributor

@chervias chervias commented Apr 29, 2024

Functions for detector splits. First we start with a relative detector function, then we can add an absolute splits, etc.

@chervias chervias requested a review from msilvafe April 29, 2024 16:50
@chervias chervias changed the title Functions for detector spl Functions for detector splits Apr 29, 2024
Copy link
Contributor

@msilvafe msilvafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments to make this a bit more compatible with the structure used in other functions and to be more compatible with the structure of the preprocess module. Perhaps @kmharrington or @mhasself has opinions about whether this should go in a new obs_ops module or if it should go in tod_ops.flags I suppose since these are returning boolean masks that are shape dets not RangesMatrices shape dets x samps it may make more sense to delineate that way and have dets type masks live here in obs_ops

import numpy as np
from sotodlib.core import FlagManager

def det_splits_relative(aman, det_left_right=False, det_upper_lower=False, det_in_out=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an argument called wrap which defaults to None and if None or False it does not wrap the flags into the input axis manager. If wrap is a True it wraps into some default field name det_flags and if wrap is a string then it wraps the flags into that field name.

Comment on lines 25 to 28
try:
aman.det_flags
except KeyError:
aman.wrap('det_flags', FlagManager.for_tod(aman))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a new axis manager called det_flags or dflag_aman and only wrap it if the wrap argument is not False/None.

aman.det_flags.wrap_dets('det_in', np.logical_not(mask))
mask = radii > radius_median
aman.det_flags.wrap_dets('det_out', np.logical_not(mask))
return aman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also return the det_flags or dflag_aman axis manager and only wrap it into aman if wrap is not None/False.

@chervias chervias requested a review from msilvafe April 29, 2024 23:27
@chervias
Copy link
Contributor Author

If I try to run again the function and a wrap is attempted for a field that already exists this will fail, but I think it should overwrite.

Copy link
Contributor

@msilvafe msilvafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, the issue you brough up about overwriting I addressed in PR #824 and I don't think should be tied to this PR.

Actually before merge can you add some docs into /docs perhaps add an obs_ops.rst and describe the purpose of the splits and how to use there?

mask = radii > radius_median
fm.wrap_dets('det_out', np.logical_not(mask))

if wrap is not None and wrap is not False:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be if not(wrap) if wrap is None or False then not(wrap) will be True.

aman.wrap('det_flags', fm)
else:
aman.wrap(wrap, fm)
return aman, fm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return fm don't need to return aman

A couple of minor fixes
@chervias
Copy link
Contributor Author

done. So for example if I wrap a left/right split, and later I decide I actually want a left/right and a upper/lower split, when I run this function again it will fail. I think it should by default overwrite. Do you agree? We can add that later once there is a clean way of overwriting fields to a aman

@chervias
Copy link
Contributor Author

sorry I didn't see docs stuff, I'll do it before merging

@msilvafe
Copy link
Contributor

done. So for example if I wrap a left/right split, and later I decide I actually want a left/right and a upper/lower split, when I run this function again it will fail. I think it should by default overwrite. Do you agree? We can add that later once there is a clean way of overwriting fields to a aman

Yes for now outside of this function you will need to run aman.move('det_flags', None) and can move overwrite in here if needed when that PR is merged.

Copy link
Contributor

@msilvafe msilvafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple comments but won't make them blocking. So you know, to test the docs build I create a virtual environment, install all of the dependencies, and then build the html.

$ virtualenv test
$ source test/bin/activate
$ pip install -r docs/requirements.txt
$ cd docs
$ make html

Then I just open the html files written to sotodlib/docs/_build/html and inspect the relevant pages to make sure all the formatting looks reasonable. When building this way Warnings don't get elevated to Errors like they do in the GitHub CI tests (which was what you pinged me about).

docs/obs_ops.rst Outdated
Comment on lines 26 to 29
---------

.. autofunction:: sotodlib.obs_ops.splits.det_splits_relative
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete these lines, the line above that calls :members: means that it will render the docstrings for all of the functions within the obs_ops.splits module so currently you're duplicating the docstring for that fn in two places:
Screenshot 2024-05-01 at 10 21 02 AM

@@ -55,8 +55,10 @@ def det_splits_relative(aman, det_left_right=False, det_upper_lower=False, det_i

if not(wrap):
if wrap == True:
aman.move('det_flags', None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if det_flags doesn't exist in aman already this will fail so may need a check like:

if name in aman._fields:
    aman.move(name, None)

@chervias chervias requested a review from msilvafe May 1, 2024 19:21
Copy link
Contributor

@msilvafe msilvafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks Carlos.

@chervias chervias merged commit 37201ff into master May 1, 2024
5 checks passed
@chervias chervias deleted the add_det_splits branch May 1, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants