search

hpc4cmb / toast

Time Ordered Astrophysics Scalable Tools
Other
44 stars 39 forks source link

Common mode fix #665

Closed giuspugl closed 1 year ago

giuspugl commented 1 year ago

with this PR we aim at fixing the issue on-going with the common mode operator, https://github.com/hpc4cmb/toast/issues/661 to also make sure that the error will be caught up in the future i've also included an extra-test.

giuspugl commented 1 year ago

it seemed like @keskitalo was maybe intending to specify a smaller set of detectors as a trait or somewhere else?

then i am a bit confused, isn't enough to specify this with self.focalplane_key ?https://github.com/hpc4cmb/toast/blob/0ca8204cb7a5087bcb881f876ebe11c6caa6d270/src/toast/ops/common_mode_noise.py#L46

keskitalo commented 1 year ago

Thank you for the PR. The detset trait is indeed missing from the operator. The idea for the operator was this:

  1. If focalplane_key is set, then create common modes between detectors that share that key
  2. If there is no focalplane_key, then the default is to create common modes between all detectors
  3. If there is no focalplane_key but the user provided detset, then add a common mode only between the specified detectors.

This is not captured in the doc strings because I forgot to add the detset trait. Let me edit the branch directly to add the trait.

Thanks for the unit test!

keskitalo commented 1 year ago

@giuspugl Please have a look at the changes I just pushed.

giuspugl commented 1 year ago

thanks a lot @keskitalo ! i am happy with these changes !