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

Baseline-dependent rebinner #269

Merged
merged 8 commits into from
May 27, 2024
Merged

Baseline-dependent rebinner #269

merged 8 commits into from
May 27, 2024

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented May 15, 2024

Summary

Implements a rebinning task similar to #266. This updated version uses the actual per-baseline weights, rather than a mean, and avoids creating a separate rebin_weight dataset entirely. This should provide marginally better rebinning behaviour in the presence of bad baselines, and does not require complicated tracking of multiple datasets throughout various pipelines.

It also implements a baseline-dependent effective_ra dataset and a simplified effective_ra centring correction.

From testing, this seems to show a light reduction in noise across all delays compared to the mean-weighted rebinning.

Notes

  • The rebinning has been tested through the chime daily pipeline
  • Gradient correction has been tested
  • Stacking of daily data has not been tested, but is assumed to be ok given no issues with daily products
  • The addition of a stack axis in the effective_ra dataset means that containers produced by the old rebinner cannot be loaded with this PR

This PR will replace #266 and would close chime-experiment/ch_pipeline#242 as it is no longer needed.

Depends on radiocosmology/caput#239 and #231 for updates to container copying behaviour. I've decided to implement this in a future PR

@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch from a255612 to 61ea69b Compare May 15, 2024 21:34
@ljgray ljgray marked this pull request as ready for review May 15, 2024 22:58
@ljgray ljgray requested review from ssiegelx and sjforeman May 15, 2024 22:58
Copy link
Contributor

@ssiegelx ssiegelx 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 great and simplifies things considerably compared to the baseline-independent weighting. I just have a few comments.

draco/analysis/sidereal.py Outdated Show resolved Hide resolved
draco/analysis/sidereal.py Outdated Show resolved Hide resolved
draco/analysis/sidereal.py Outdated Show resolved Hide resolved
draco/analysis/sidereal.py Outdated Show resolved Hide resolved
draco/core/containers.py Outdated Show resolved Hide resolved
draco/analysis/sidereal.py Outdated Show resolved Hide resolved
@sjforeman sjforeman requested a review from albinje May 20, 2024 16:34
@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch 6 times, most recently from ecda38d to a5826f9 Compare May 22, 2024 19:03
@ljgray ljgray requested a review from ssiegelx May 22, 2024 19:03
@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch 3 times, most recently from 2acb3bb to 4dcfa25 Compare May 22, 2024 21:34
@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch 2 times, most recently from 26cd861 to e9cb12f Compare May 24, 2024 18:33
draco/util/regrid.py Outdated Show resolved Hide resolved
@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch from e9cb12f to d54e3b2 Compare May 24, 2024 21:59
@ljgray ljgray requested a review from ssiegelx May 24, 2024 22:00
@ljgray ljgray force-pushed the lg/multi-baseline-rebinner branch from d54e3b2 to f800c5a Compare May 24, 2024 23:04
Copy link
Contributor

@ssiegelx ssiegelx 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!

@ljgray ljgray merged commit 37b4184 into master May 27, 2024
4 checks passed
@ljgray ljgray deleted the lg/multi-baseline-rebinner branch May 27, 2024 18:21
@ljgray ljgray mentioned this pull request May 27, 2024
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