search

HERA-Team / hera_cal

Library for HERA data reduction, including redundant calibration, absolute calibration, and LST-binning.
MIT License
13 stars 8 forks source link

Add an option to be able to ignore N-sample while LST-binning #932

Closed Kai-FengChen closed 2 months ago

Kai-FengChen commented 6 months ago

Add an option weight_by_nsamples (default True). When turning off, will weight only by flagging patterns but still propagate the nsamples.

(Minor concern: Is it a bad practice to set something with default True? When modifying the arg_parser it feels a bit weird to have an argument that has action="store_true" but also default to be True... But in my defence, weight_by_nsamples seems more straightforward than not_weight_by_nsamples)

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.18%. Comparing base (cc0a13d) to head (b1288c5).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #932 +/- ## ========================================== - Coverage 97.18% 97.18% -0.01% ========================================== Files 30 30 Lines 10733 10727 -6 ========================================== - Hits 10431 10425 -6 Misses 302 302 ``` | [Flag](https://app.codecov.io/gh/HERA-Team/hera_cal/pull/932/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/HERA-Team/hera_cal/pull/932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team) | `97.18% <ø> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Kai-FengChen commented 6 months ago

Yes so the idea is because we now keep track of N-samples, channels that are originally flagged and inpainted will still have zero nsample. If we used the original lstbin routine, the inpainted channel will be weighted by 0 during lstbining. By changing to this only_weighted_by_flag option we can average inpainted data with real data during lstbining.

Kai-FengChen commented 2 months ago

After a quick discussion with @jsdillon, I removed the tester test_lstbin.py for the old lst binner and resolved some conflicts to have this PR ready to be merged.

As a reminder, this PR is for the H4C re-run so that we are able to properly propagate nsample (i.e., treat inpainted channels as having nsample 0) but still use inpainted data during lst binning (so weight data by flagging pattern instead of nsample).