-
Notifications
You must be signed in to change notification settings - Fork 19
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
Adding AZSS correction per subscan #1065
base: master
Are you sure you want to change the base?
Conversation
…pdated noise logic to incorporate wrap_name from config\n\- added handling for 'fknee' and 'min_noise' in noise process based on user-defined thresholds
71891d7
to
6d888d9
Compare
…stimation from get_azss with flag subscan, TODO: azss subtraction per subscan still problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some inline comments.
sotodlib/preprocess/processes.py
Outdated
def calc_and_save(self, aman, proc_aman): | ||
if 'flags' not in proc_aman._fields: | ||
from sotodlib.core import FlagManager | ||
proc_aman.wrap('flags', FlagManager.for_tod(proc_aman)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we don't wrap into proc_aman
directly in calc_and_save
instead we make one new AxisManager here (say bss_aman
--bad sub scan aman), wrap everything into that, and then pass that to save
(in the field typically called calc_aman
) and the save method wraps that into proc_aman
so that everything for this one preprocess step is in a single field in proc_aman. Here you wrap into proc_aman directly and also wrap ss_aman, and det_aman all into separate fields of proc_aman. Let's put everything in one aman and send just that one to save
.
sotodlib/preprocess/processes.py
Outdated
subscan_stats_T = proc_aman[self.stats_name+"_T"] | ||
subscan_stats_Q = proc_aman[self.stats_name+"_Q"] | ||
subscan_stats_U = proc_aman[self.stats_name+"_U"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this requires that you run GetStats
three times wrapping in the same name but appending _T
, _Q
, and _U
to the end? That's a pretty specific prerequisite. Plus I think 90% of the time we are running GetStats
it'll be to achieve what BadSubscanFlags
is achieving (probably the other 10% of the time is getting some stats on the full TOD or PSD I guess). So why don't we instead check if self.stats_name+"_T"
exists and if not call tod_ops.flags.get_stats to get the info you need.
sotodlib/tod_ops/flags.py
Outdated
@@ -998,3 +1021,73 @@ def noise_fit_flags(aman, low_wn, high_wn, high_fk): | |||
return flag_valid_fk | |||
else: | |||
return None | |||
|
|||
def get_noisy_subscan_flags(aman, subscan_stats_T, subscan_stats_Q, subscan_stats_U, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this require T, Q, and U simultaneously makes this a very restrictive function (i.e. can only be run for the SATs not LAT and only after demodulation). Maybe we can make this take just one signal (i.e. signal = subscan_stats_T) and produce one dets x subscan boolean mask/RangesMatrix per signal then the preprocess method could choose how to combine these i.e. if there's one use 1, if there's >1 use logical and or logical or, etc. Thoughts?
sotodlib/tod_ops/flags.py
Outdated
raise ValueError(f"Flag name {name} already exists in aman.flags") | ||
aman.flags.wrap(name, noisy_subscan_flags) | ||
|
||
return noisy_subscan_flags, noisy_detector_flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix newline
sotodlib/preprocess/processes.py
Outdated
def process(self, aman, proc_aman): | ||
subtract = self.process_cfgs.get('subtract', False) | ||
if self.proc_aman_turnaround_info: | ||
_f = attergetter(self.proc_aman_turnaround_info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: attrgetter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
sotodlib/preprocess/processes.py
Outdated
if 'flags' not in proc_aman._fields: | ||
from sotodlib.core import FlagManager | ||
proc_aman.wrap('flags', FlagManager.for_tod(proc_aman)) | ||
proc_aman.flags.wrap("left_scan", aman.flags.left_scan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left_scan and right_scan are already in the proc_aman after FlagTurnarounds, just top-level not under "flags". We probably want to agree on one place for them and just look there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nominally we shouldn't be wrapping anything into aman unless it's explicitly intended to be done in a process
step and then merged into proc_aman
in the calc_and_save
step. However the wrapping into aman is happening in calc_and_save
for FlagTurnarounds which should not be allowed. Long story short we should have all of the merge options hard coded false in the call to get_turnaround_flags
in calc_and_save
and any functions that look for info from TurnaroundFlags should look for it in proc_aman.
sotodlib/tod_ops/azss.py
Outdated
right_mask = turnaround_info.right_scan | ||
else: | ||
print("") | ||
print("HEREEEEEEE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover debug statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments/questions on formatting and organization.
sotodlib/preprocess/processes.py
Outdated
tod_ops.azss.get_azss(aman, azss_stats_name=self.azss_stats_name, | ||
turnaround_info=turnaround_info, | ||
merge_stats = True, merge_model=False, | ||
subtract_in_place=subtract, **self.calc_cfgs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably has already been discussed so feel free to ignore this, but this is using calc_cfgs
within process
which seems a bit counter-intuitive. I think it might be unavoidable given the way this needs to be run, but maybe these would make more sense as process_cfgs
since process
is always going to be run and will run first.
@@ -313,15 +435,31 @@ def subtract_azss(aman, signal='signal', azss_template_name='azss_model', | |||
else: | |||
raise TypeError("Signal must be None, str, or ndarray") | |||
|
|||
azss_stats, model_left, model_right = get_model_sig_tod(aman, azss_stats, az=None) | |||
|
|||
if in_place: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional, but I think this logic can be simplified by assigning to a temporary variable, i.e.:
if in_place:
if signal_name is None:
sig = signal
else:
sig = aman[signal_name]
else:
if signal_name is None:
sig = signal.copy()
else:
sig = aman[signal_name].copy()
As long as you use sig -= mask
, the non-copy ones should stay as references.
if self.azss_stats_name in proc_aman: | ||
if subtract: | ||
tod_ops.azss.subtract_azss(aman, proc_aman[self.azss_stats_name], | ||
signal = self.calc_cfgs.get('signal'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor formatting on signal = self.calc_cfgs.get('signal')
(space not needed).
sotodlib/preprocess/processes.py
Outdated
else: | ||
tod_ops.azss.get_azss(aman, azss_stats_name=self.azss_stats_name, | ||
turnaround_info=turnaround_info, | ||
merge_stats = True, merge_model=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want merge_model
hardcoded to False
here (or elsewhere)? Is there any case where we'd want to be able to look at the model after fitting or interpolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because it wraps in a n_dets x n_samps copy of the data, we can use get_model_sig_tod
to reconstruct it from the info in azss_stats
if we need to look at it later.
sotodlib/preprocess/processes.py
Outdated
else: | ||
tod_ops.azss.get_azss(aman, azss_stats_name=self.azss_stats_name, | ||
turnaround_info=turnaround_info, | ||
merge_stats = True, merge_model=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting with unneeded spaces on merge_stats
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, added some new comments
@@ -741,7 +717,7 @@ def get_inv_var_flags(aman, signal_name='signal', nsigma=5, | |||
|
|||
return mskinvar | |||
|
|||
def get_subscans(aman, merge=True, include_turnarounds=False, overwrite=True): | |||
def get_subscans(aman, flags="flags", merge=True, include_turnarounds=False, overwrite=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove new "flags" argument that doesn't get used in this function
@@ -767,6 +743,7 @@ def get_subscans(aman, merge=True, include_turnarounds=False, overwrite=True): | |||
""" | |||
if not include_turnarounds: | |||
ss_ind = (~aman.flags.turnarounds).ranges() # sliceable indices (first inclusive, last exclusive) for subscans | |||
#ss_ind = (~aman[flags]["turnarounds"]).ranges() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray comment
@@ -1022,13 +999,18 @@ def noise_fit_flags(aman, low_wn, high_wn, high_fk): | |||
else: | |||
return None | |||
|
|||
def get_noisy_subscan_flags(aman, subscan_stats_T, subscan_stats_Q, subscan_stats_U, | |||
def get_noisy_subscan_flags(aman, subscan_stats, flags="flags", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused flags argument (or use it)
nstd_lim=None, ptp_lim=None, kurt_lim=None, | ||
skew_lim=None, merge=False, overwrite=False, | ||
name="bad_subscan", cut_bad=False): | ||
skew_lim=None, noisy_detector_lim=0.9, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add noisy_detector_lim to dosctring and replace "50%" in other comments below
|
||
if "subscan_info" in aman: | ||
ssf = aman.subscan_info.subscan_flags | ||
elif "subscans" in aman: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need this block? After FlagTurnarounds the subscans should be in subscan_info.subscan_flags for both the aman and proc_aman. proc_aman.subscans is the subscans axis
return noisy_subscan_flags, ~noisy_detector_flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to invert here I would add a comment to the effect of "True for good dets" or something and put the same thing clearly in the docstring (should prob have that whether you invert or not)
@@ -49,7 +49,7 @@ def get_qu_common_mode_coeffs(aman, Q_signal=None, U_signal=None, merge=False): | |||
return output_aman | |||
|
|||
def subtract_qu_common_mode(aman, Q_signal=None, U_signal=None, coeff_aman=None, | |||
merge=False): | |||
merge=False, subtract=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused param (or use it and add to docstring)
@@ -273,7 +274,9 @@ def get_azss(aman, signal='signal', az=None, frange=None, bins=100, flags=None, | |||
|
|||
if flags is None: | |||
flags = Ranges.from_mask(np.zeros(aman.samps.count).astype(bool)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to assume flags is (dets, samps) lower down so will this work?
ss_aman = core.AxisManager(aman.dets, aman.samps) | ||
ss_aman.wrap("valid_subscan", ~msk_ss, [(0, 'dets'), (1, 'samps')]) | ||
ss_aman.wrap("valid_subscans", msk_ss, [(0, 'dets'), (1, 'samps')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is True for bad subscans. As such "valid_subscans" is a somewhat confusing name. Perhaps change to "bad_subscans" or something, or at least add a comment.
This is doubly confusing since valid_dets is True for good detectors. Would be good to use the same convention (which I think is to use True for bad ones)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, by default in sotodlib the flags scheme is False for the stuff you want to keep and True for the stuff you want to cut. That is why the parameter in the pointing matrix functions is cuts=
, where it expects a flag where the stuff you want to cut is True. This also explains why the union of flags functions is a union (logical OR) and not an intersection (logical AND). So always keep this in mind when creating flags.
Addresses remaining part of issue #1009, i.e. adding AZSS correction per subscan, as a continuation of the PR in #1064.