Median nightly delays#795
Conversation
Codecov Report
@@ Coverage Diff @@
## main #795 +/- ##
==========================================
+ Coverage 97.04% 97.07% +0.03%
==========================================
Files 18 18
Lines 8294 8389 +95
==========================================
+ Hits 8049 8144 +95
Misses 245 245
Continue to review full report at Codecov.
|
|
I'm surprised that last step does literally nothing... Have you checked multiple antennas? What's your reference antenna? We saw jumps like these in H1C IDR2 before there was ever any polarity flipping, e.g. figure 6 of https://arxiv.org/pdf/2108.02263.pdf |
The last plot had a bug. This is actually what happens when you try to fix the delays without paying attention to polarity flips and then flip polarities as a separate step. |
|
I've updated this PR so that the default recipe for updating first-cal is to both switch the polarities and to update the offsets and delays in the degenerate subspace. I found that the polarity flips fixes the polarity misidentification. Updating the degeneracies, while it has minimal impact on the |
| ---------- | ||
| redcal_meta_file_list: list of str | ||
| list of file names containing redcal meta data. All files should have same number of times. | ||
|
|
There was a problem hiding this comment.
Please document the other kwargs, even though they are pretty obvious.
| output_replace='.redcal_meta.hdf5'): | ||
| """ | ||
| Find the median delay and polarity for list of firstcal meta files. | ||
| and write them out with that median. |
There was a problem hiding this comment.
write them out with that median. is unclear. What you're actually doing is making new redcal meta files with median-ed info.
Also, this function is misleadingly named, since it also edits offsets (missing from the docstring) and polarities. Perhaps just call it median_nightly_firstcal_metadata or similar?
| redcal_metas = [] | ||
| for meta_file in redcal_meta_file_list: | ||
| redcal_metas.append((meta_file, ) + read_redcal_meta(meta_file)) |
There was a problem hiding this comment.
I would have used a list comprehension
| # compute medians. | ||
| for ant in delays: | ||
| delays[ant] = np.nanmedian(delays[ant]) | ||
| polarity_flips[ant] = bool(np.nanmedian(polarity_flips[ant])) |
There was a problem hiding this comment.
If all the polarity flips are np.nan for a given antenna, this will turn into True though I think you want it to be False by default. Probably doesn't matter but you might want an a check on not np.any(np.isfinite())
| # get redundant calibrator. | ||
| # get reds |
There was a problem hiding this comment.
I think these comments got misplaced
| path to output file. Optional to write output to. | ||
| replace_degens: bool, optional | ||
| If True, replace degenerate portion of calibration solution with solution in redcal_meta_file. | ||
| fix_polarities: bool, optional |
There was a problem hiding this comment.
If you have this option, it's weird that old_redcal_meta_file is a required parameter. Perhaps you want to put a way to allow the user to pass None for old_redcal_meta_file if this is False?
| if not dont_replace_polarities: | ||
| for ant in fc_meta['polarity_flips']: | ||
| gains[ant] *= (-1. + 0j) ** np.abs(fc_meta['polarity_flips'][ant][:, None] - fc_meta_old['polarity_flips'][ant][:, None]) | ||
| ntimes = hc.Ntimes |
| ntimes = hc.Ntimes | ||
| if not dont_replace_degens: | ||
| for ant in gains: | ||
| dly_factor = np.exp(2j * np.pi * freqs[None, :] * fc_meta['dlys'][ant][:, None]) * np.exp(1j * fc_meta['offsets'][ant][:, None]) |
There was a problem hiding this comment.
a bit weird to call it a dly_factor when it includes an offset (which can be summed inside the exponent)
| redcal_opts.add_argument("--upsample", default=False, action="store_true", help="Upsample BDA files to the highest temporal resolution.") | ||
| redcal_opts.add_argument("--downsample", default=False, action="store_true", help="Downsample BDA files to the highest temporal resolution.") | ||
| redcal_opts.add_argument("--pol_mode", type=str, default='2pol', help="polarization mode of redundancies. Can be '1pol', '2pol', '4pol', or '4pol_minV'. See recal.get_reds documentation.") | ||
| redcal_opts.add_argument("--pol_mode", type=str, default='2pol', help="polarization mode of redundancies. Can be '1pol', '2pol', '4pol', or '4pol_minV'. See redcal.get_reds documentation.") |
| # check that files have been created | ||
| for ofile in omnifiles: | ||
| assert os.path.exists(omni_file.replace('.calfits', '.medphase.calfits')) | ||
|
|
There was a problem hiding this comment.
Is there any other test we can do? Like, does doing this make the calibration solutions smoother in time? If so, can we quantify that and make an assertion about it?
There was a problem hiding this comment.
I think it's overkill to show whether or not the calibration is smoother. Anecdotally, I find that the differences in the solutions after replacing the degeneracies are quite small. I will document this above.
There was a problem hiding this comment.
Fair point. Mostly I'm interested in the question of whether polarities are handled appropriately.
|
I am unable to reproduce the macOS unittest failures on a mac. I have tried to create a fresh conda environment and still no luck. Should we remove the macOS tests? |
|
Maybe @bhazelton or @plaplant have ideas about how to debug testing failures that appear only to happen in the CI but can't be reproduced locally... |
|
@aewallwi @jsdillon I ran the tests locally on my (M1-based) Mac in a fresh environment, and I'm getting a test failure too, though on a different test than the CI failures. I've posted the full traceback at the bottom of my comment. As a way to debug testing failures in CI that don't appear locally, I tend to ssh into the machine running the job, and poke around on the command line there. This way you're using the exact same environment and hardware that produced the error, so it's easier to figure out what's going wrong. I would highly (can't stress this enough) recommend against ignoring test failures or tuning off CI checks. It's better in the long run to get to the bottom of failing tests, and understand what's going wrong. My local test failure: |
|
Thanks @plaplant. How do you ssh into github actions? |
…hera_cal into median_nightly_delays
for more information, see https://pre-commit.ci
…hera_cal into median_nightly_delays
The tests pass for linux which is the OS that is going to be used for our science runs. I think this raises an interesting question. How much time do we want to be investing in order to fully support multiple operating systems when Linux is the standard for our production analysis and the error here is not reproducible on other macOS machines and could potentially be a bug on githubs end? |
…xis to perform restorative padding.





Tools to better ensure that redcal degeneracies are relatively constant in time. Depends on #772. Depends on #798 #815 . Coverage gaps arise from gaps that will be addressed in some shape or form in #798