Closed r-pascua closed 2 years ago
Merging #212 (75f95c6) into main (ce9010e) will increase coverage by
0.07%
. The diff coverage is100.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%) |
:arrow_up: |
hera_sim/noise.py | 100.00% <100.00%> (+2.04%) |
:arrow_up: |
hera_sim/simulate.py | 94.82% <100.00%> (+0.17%) |
:arrow_up: |
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.
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.
As proof that this is working better, here's some plots for data simulated with this code:
BEFORE:
AFTER:
@aewallwi I think you want to post that stuff over on #213. I'd like to separate out the question of getting
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.
@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 I think you want to post that stuff over on #213. I'd like to separate out the question of getting
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
Issue about generality and complexity is posted (#214). Merging and closing this PR now.
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 tohera_sim
.Edit: Many tests broke as a result of recent changes to
vis_cpu
. I've also fixed those tests in this PR.