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

Adds force_pointing kw to match_params #924

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jlashner
Copy link
Contributor

  • New force_pointing match param that will make it so only detectors with pointing information are matched.
  • Removes the force_src_pointing param (which would force a match if pointing is provided) since this was always buggy and never had good results
  • Allows you to set resonator pointing from the ResSet.from_aman function.

This which will only match resonators that have pointing information provided.
This also adds the ability to set resonator pointing from the `from_aman` function.
@jlashner jlashner requested a review from mhasself August 12, 2024 15:14
Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

As discussed I think a more complete matching rule for incorporating strict position info would need to consider the detector type:

  • NC: do not match (these don't have design_freq anyway)
  • DARK, SLOT: may or may not have position meas.
  • UNRT, SQID, BARE: must not have position meas.
  • OPTC: must have position meas.

@jlashner
Copy link
Contributor Author

jlashner commented Sep 9, 2024

Alright, I added different pointing checks that depend on the det-type like you described above, and confirmed that it still works and that I still get good matches. Info on the new match solutions + plots are here: https://simonsobs.atlassian.net/wiki/x/RwC2JQ

I also changed the config option from force_pointing to enforce_pointing_reqs.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

Also remove .vscode files!

Comment on lines 561 to 568
def get_det_type_mask(arr: np.ndarray, det_types: List[str]) -> np.ndarray:
"""
Returns a boolean mask of all dets that have specific detector types.
"""
return np.logical_or.reduce([
arr['det_type'] == dt
for dt in det_types
])
Copy link
Member

Choose a reason for hiding this comment

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

I think you are looking for np.isin!

mat[m, :] = np.inf
m = np.isnan(dst_arr['xi']) | np.isnan(dst_arr['eta'])
mat[:, m] = np.inf
if self.match_pars.enforce_pointing_reqs:
Copy link
Member

Choose a reason for hiding this comment

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

So in this block, 'DARK' and 'SLOT' are implicitly untouched. I think you need to at least comment on DARK/SLOT, if not check explicitly that everything you haven't directly modified is listed as DARK or SLOT.

Comment on lines 590 to 592
enforce_pointing_reqs (bool):
If True, will enforce pointing requirements that depend on resonator
type.
Copy link
Member

Choose a reason for hiding this comment

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

Either here, or somewhere else, explain in words what those requirements are (i.e. OPTC must have a position match, DARK/SLOT may have a position match, and everything else must not).

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.

2 participants