New Crosstalk Model

[r-pascua]

This PR implements an improved crosstalk model, meant to better replicate crosstalk seen in H1C observations. Some of the Simulator code had to be modified (in particular, stuff to do with parsing the model signature and pulling parameters from the Simulator object as needed), but most of the changes are in the sigchain module.

I still need to write tests and document the new class, but I figure we could start the review process now.

Here's an example of simulated crosstalk (on top of foreground+eor visibilities) at a single time, based on the IDR2 validation sims.

[codecov[bot]]

Codecov Report

Merging #185 (cdcc356) into main (de7bd30) will increase coverage by 1.65%. The diff coverage is 95.44%.

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   94.91%   96.56%   +1.65%     
==========================================
  Files          24       24              
  Lines        2755     2769      +14     
==========================================
+ Hits         2615     2674      +59     
+ Misses        140       95      -45     

Impacted Files	Coverage Δ
hera_sim/simulate.py	94.64% <90.90%> (-0.30%)	:arrow_down:
hera_sim/visibilities/simulators.py	87.87% <92.03%> (+12.52%)	:arrow_up:
hera_sim/sigchain.py	98.29% <96.29%> (-0.29%)	:arrow_down:
hera_sim/visibilities/healvis_wrapper.py	93.24% <97.82%> (+4.71%)	:arrow_up:
hera_sim/visibilities/vis_cpu.py	93.38% <97.89%> (+5.72%)	:arrow_up:
hera_sim/beams.py	98.20% <100.00%> (+1.34%)	:arrow_up:
hera_sim/io.py	98.59% <100.00%> (+0.04%)	:arrow_up:
hera_sim/visibilities/__init__.py	100.00% <100.00%> (+28.57%)	:arrow_up:
hera_sim/visibilities/pyuvsim_wrapper.py	100.00% <100.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 19f8384...cdcc356. Read the comment docs.

[jsdillon]

Happy to take a look. LMK when you two are converged.

[r-pascua]

Thanks @r-pascua, it's hard for me to grok the physics here, but the code looks good (just a couple of suggestions). Obviously still needs docs/tests. I'd also prefer if someone like @jsdillon would also review.

Thanks for the review! I'll make the requested changes soon. In the docs I'll put the math describing what's going on. The basic idea is that the crosstalk on baseline (i,j) has contributions from the two autos (i,i) and (j,j) with their own reflection coefficients. The crosstalk mechanism is modeled as the autos traveling down the cable to the receiverator and then transmitting over the air back to the antennas (in the sense that signal from antenna i travels to the receiverator then gets broadcast to antenna j, and vice-versa), so the reflection coefficient amplitudes have some distance dependence and the delays come from cable travel time + overair transmit time (plus some offset, but I haven't included that as a parameter). A bit of a caveat is that this model only describes the primary peak in the crosstalk—the extra peaks are kind of just thrown in there in a way that produces something that looks like what we see in the data. I'll do my best to document this clearly both in the docstring and with inline comments in the code.

[steven-murray]

OK cool thanks for the explanation! I wonder, for the "extra bits" that are attached to the main peak -- if they're not physically from the same source, should they be a different Model? The user can then choose to add either or both.

[jsdillon]

They are probably from the same source, they are just probably some sort of multi-path over the air effect. We don't really know what they are, but we know that you don't get single deltas without the "extra bits" in any of our H1C data, so I'd be hesitant to separate the two.

[r-pascua]

OK, sorry for the delay, but I believe this is now ready for review. I've fleshed out the docs and written a (somewhat simple) test to make sure it works. I think I want to leave the change to the Simulator (in particular, how it parses class __init__ and __call__ methods to figure out what parameters to pass) to another PR.

@steven-murray and @jsdillon, if you could give a review when you have time, then that would be great!

[r-pascua]

@jsdillon thanks for the review! I was thinking it would be good to perform the test you suggest. I'll see about running that test when I have a bit of free time. What do you think about adding a delay_offset parameter?

[jsdillon]

@jsdillon thanks for the review! I was thinking it would be good to perform the test you suggest. I'll see about running that test when I have a bit of free time. What do you think about adding a delay_offset parameter?

I explored whether there was any good evidence for a delay_offset in the memo and I'm not convinced it leaving it as a free parameter made the fit appreciably better. I think we can leave it out of this simulator.

[r-pascua]

Ok, gotcha. Would you like me to run the simulation on an array before merging this in? I also need to do the typing for the new class parameters, so I won't merge just yet.

[jsdillon]

I leave that to you. I think making that a prerequisite of pulling in this PR is a somewhat selective demand for rigor. If you feel like it would build confidence you want to have, go for it. I don't think it has to be part of the unit_tests, of course, but if you do it, definitely post plots here.