fix: only add Trx bias to autocorr noise

[r-pascua]

This PR makes sure that autocorrelations only receive a receiver temperature bias when using the ThermalNoise class to simulate noise. This is exactly what we did for H1C IDR2 validation, but I forgot to transfer it over to hera_sim.

Edit: Many tests broke as a result of recent changes to vis_cpu. I've also fixed those tests in this PR.

[codecov]

Codecov Report

Merging #212 (75f95c6) into main (ce9010e) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   96.48%   96.56%   +0.07%     
==========================================
  Files          24       24              
  Lines        2792     2797       +5     
==========================================
+ Hits         2694     2701       +7     
+ Misses         98       96       -2     

Impacted Files	Coverage Δ
hera_sim/visibilities/vis_cpu.py	93.38% <ø> (ø)
hera_sim/components.py	91.91% <100.00%> (+0.08%)	⬆️
hera_sim/noise.py	100.00% <100.00%> (+2.04%)	⬆️
hera_sim/simulate.py	94.82% <100.00%> (+0.17%)	⬆️

---

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 9576e78...75f95c6. Read the comment docs.

[r-pascua]

Posting an update to detail the problem and describe the solution I implemented.

It was found that the Simulator was not properly simulating noise, and this was revealed by comparing plots of the noise estimated from the data to the noise expected from the autos. It turns out that the issue was that the Simulator wasn't actually setting the autovis parameter to anything, since the _initialize_args_from_model method wasn't tracking this parameter. The _initialize_args_from_model method used to only track arguments without default values, and so it would entirely miss the autovis parameter in the ThermalNoise model, since it's optional. I've updated the logic of the _initialize_args_from_model method to also include parameters that are specified in a new class attribute common to all SimulationComponent subclasses that I've named _extract_kwargs. In practice, this is just something of a hotfix for the issue that maintains backwards compatibility with the ThermalNoise class and does not introduce any changes to the API. The idea is that this is a prototype for a feature @steven-murray and I have discussed on multiple occasions, where the model classes inform the Simulator of which parameters it should pull from the data. I say that it is a prototype because I think that the proper implementation should not only tell the Simulator which of the model arguments should be extracted from the data, but it should also tell the Simulator how to extract those values, to the extent possible. (Of course, this stance is completely up for debate. It may turn out that the simplest thing to do is to just update the logic in the Simulator._update_args method whenever a new model class is introduced that adds a unique entry to the set of all _extract_kwargs entries.)

Anyway, this turned out to be a somewhat deeper and more nuanced issue than I originally thought it was, so I would like @steven-murray to take a look, as well as @jsdillon.

[jsdillon]

This all looks good to me.

I will say that, in general, that the way this was all coded makes it very difficult to figure out what was passed into, for example, ThermalNoise.__init__(). There's a tradeoff here between generality/extensibility and clarity and I worry we've gone too far toward the former two. Getting parameters as *args and **kwargs from inspecting class constructor signatures is clever, but I worry its a lot harder to follow than just passing things around explicitly. Anyway, no specific suggestions on this front for now, other than a vague suggestion for where this might go in the future.

[jsdillon]

As proof that this is working better, here's some plots for data simulated with this code:

BEFORE:

AFTER:

[aewallwi]

Comment moved to #213 (comment)

[jsdillon]

@aewallwi I think you want to post that stuff over on #213. I'd like to separate out the question of getting <V_ii> right with the question of getting the variance of V_ii right. One is key to H1C IDR3 validation, the other is useful more broadly.

[steven-murray]

Thanks @r-pascua this looks like a good idea for now.

I agree in principle with @jsdillon that it feels like we may have strayed too much in the direction of generality while sacrificing explicitness. I know we've discussed this before, and it's a tricky problem, but perhaps we can return to it for v3.

For now, this looks like it works and makes sense.

[r-pascua]

Thanks for the review and comments, everyone. I'm realizing that the level of generality here is coming across as more of a detriment on the development side, so I think it's worthwhile to see if we can figure out a better solution for v3. I think I'd like to open an issue or discussion on this topic and try to solicit community feedback, since I don't know really know what people want out of the code or what typical use cases outside are outside of the Validation stuff I do with it.

I'll leave the PR open for a few more hours for potential responses to this comment.

[steven-murray]

@r-pascua I think an issue would be a good idea. Might be useful to tag in the issue where we previously discussed this. It's not absolutely clear to me that the current framework is not explicit enough -- that might be my own unfamiliarity with the code rather than any deficiency in the code itself. But I think its worthwhile revisiting before a new major version.

[aewallwi]

@aewallwi I think you want to post that stuff over on #213. I'd like to separate out the question of getting <V_ii> right with the question of getting the variance of V_ii right. One is key to H1C IDR3 validation, the other is useful more broadly.

I've copied this comment over to #213 and removed this one

[r-pascua]

Issue about generality and complexity is posted (#214). Merging and closing this PR now.