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

Putting the mapmaker in the same split convention as the preprocessing #1060

Merged
merged 9 commits into from
Dec 13, 2024

Conversation

chervias
Copy link
Contributor

@chervias chervias commented Dec 9, 2024

This changes how the mapmaker handles the focal plane splits, so that they follow the same convention as preprocessing.
Also fixes a minor bug in preprocess.

Copy link
Contributor

@mmccrackan mmccrackan 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.

sotodlib/preprocess/processes.py Show resolved Hide resolved
@chervias chervias requested a review from mmccrackan December 11, 2024 18:58
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is more of a belated comment on #1039 (@msilvafe) but I don't like that you must run t2p and hwpss_stats in order for any splits to be possible. This also applies to noiseQ_fit, which is not explicitly checked for. Can we make those optional? Either by silently skipping if they don't exist, or at least making some option in the config?

It would also be nice if requirements on previous preprocess steps were listed in the docstring in preprocess.processes.SplitFlags e.g. to use noise fits you must wrap into "proc_aman.noiseQ_fit" specifically.

Copy link
Contributor

@earosenberg earosenberg left a comment

Choose a reason for hiding this comment

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

Looks okay to me; would be nice to address comment on splits.py

@msilvafe msilvafe requested a review from earosenberg December 13, 2024 14:17
Copy link
Contributor

@earosenberg earosenberg left a comment

Choose a reason for hiding this comment

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

Looks great, thanks

@chervias chervias merged commit 60fb595 into master Dec 13, 2024
4 checks passed
@chervias chervias deleted the chervias/update_splits_convention branch December 13, 2024 14:38
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.

4 participants