HERA-Team / matvis

Fast matrix-based visibility simulator
MIT License
2 stars 3 forks source link

fix: correct handling of polarizations from UVBeam #29

Closed steven-murray closed 3 years ago

steven-murray commented 3 years ago

This PR updates the behaviour of vis_cpu to be more consistent. In particular, when polarized=False, we explicitly take the sqrt of the power beam for a particular feed. We raise errors for incompatible inputs.

BREAKING CHANGE: previous inputs for the beam will now sometimes raise errors, as they were inconsistent. In particular, input beam_list to vis_cpu must be efield type if using polarized=True and power type if not.

Fixes #28 Fixes #27

steven-murray commented 3 years ago

@philbull there are two remaining errors in the tests. One is for test_beam_interpolation, and it is fixed if I switch out the EllipticalBeam for a pure gaussian AnalyticBeam, so I think it has more to do with the implementation of EllipticalBeam than anything else.

The other is the comparison of vis_cpu to pyuvsim, which now is not matching well at all. I can give it a closer look, but I thought you might spot the error more quickly, since you wrote the test.

r-pascua commented 3 years ago

I figured out the problem with the comparison test—it has to do with how the beam is constructed. Since we're interpolating the UVBeam object directly, the unpolarized (power beam) case needs to take the square root of the interpolated beam (this is done in the function that grids the beam, but it isn't done in the actual call to vis_cpu). This change just needs to be made on line 253 in vis_cpu.py.

I'll see if I can figure out the other problem... The other problem wasn't actually related to the EllipticalBeam implementation, but rather the Gaussian beam case passed because none of the sources actually passed through the beam. I guess the elliptical beam had been stretched enough to allow for at least one source to pass through the main lobe, and so the test was able to pick up on the difference between the two beam interpolation schemes. It might be worth increasing the number of point sources, or futzing with the test a bit so that we still use a small number of sources, but are guaranteed to have some in the main lobe.

codecov[bot] commented 3 years ago

Codecov Report

Merging #29 (b359ed7) into main (074f3a2) will increase coverage by 0.61%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   95.72%   96.33%   +0.61%     
==========================================
  Files           5        5              
  Lines         234      273      +39     
  Branches       43       54      +11     
==========================================
+ Hits          224      263      +39     
  Misses          6        6              
  Partials        4        4              
Impacted Files Coverage Δ
src/vis_cpu/conversions.py 98.86% <100.00%> (+0.68%) :arrow_up:
src/vis_cpu/vis_cpu.py 93.87% <100.00%> (+0.26%) :arrow_up:
src/vis_cpu/wrapper.py 97.36% <100.00%> (+0.14%) :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 687cb7c...b359ed7. Read the comment docs.