Enhancement: allow external beams (plus some minor things) #43
Conversation
Add about beam inconsistency.
|
My test script is failing because it imports a couple of packages that are not available. I can work around that and will push an update. |
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
==========================================
+ Coverage 88.74% 89.19% +0.45%
==========================================
Files 16 18 +2
Lines 1661 1749 +88
==========================================
+ Hits 1474 1560 +86
- Misses 187 189 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @hughbg . Sorry I didn't see this sooner!
Thank you for adding this useful feature!
One minor issue I have is that the term "external beam" isn't very clear. It looks like this means "antenna beams" as defined in pyuvdata/sim. It's up to you, but calling it something else and adding some documentation what sort of interp function is expected would be helpful.
My main concern right now is that it still accepts the beam_pol keyword but doesn't use it if you're using antenna beams. As you can see in pyuvdata, the "polarizations" of a power beam are the various cross-pol terms, while for an E-field beam there are only different feeds to choose from.
|
|
||
| Returns: | ||
| SkyModel object | ||
| """ | ||
|
|
||
| if seed is not None: np.random.seed(seed) |
There was a problem hiding this comment.
For style's sake, this should be two lines.
| str: If it is a viable input to AnalyticBeam, | ||
| then instantiates an AnalyticBeam, otherwise assumes beam is | ||
| a filepath to a beamfits and instantiates a PowerBeam. | ||
| list: List of beam objects. This allows for external beams to be |
There was a problem hiding this comment.
I'm not a fan of the term "external" beam here. I think what you mean are the beam classes defined in pyuvdata and pyuvsim?
Since they're treated as antenna beams, it might be better to refer to them as such.
There was a problem hiding this comment.
I'll change to antenna beams. We've been using pyuvsim AnalyticBeam but mostly the new PolyBeam types in hera_sim that are derived from AnalyticBeam.
|
|
||
| if isinstance(beam, list): self.beam = beam |
There was a problem hiding this comment.
If statements should be multi-line.
It also might be good to check here that the beam object has an interp method.
| beam_val[i] = bv | ||
| for bi, bl in enumerate(self.array): | ||
| # Multiply beam correction for the two antennas in each baseline | ||
| beam_cube[bi] = beam_val[bl.ant1]*beam_val[bl.ant2] |
There was a problem hiding this comment.
Shouldn't one be complex-conjugated? Please ignore if I'm wrong about that.
There was a problem hiding this comment.
Yes I should fix that. All the beams we've used so far return real values but in general they could be complex.
| """ | ||
| interp_data, interp_basis_vector = beam.interp(az_array=az_arr, za_array=za_arr, | ||
| freq_array=freqs) | ||
| return interp_data[0, 0, 1].T # just want Npix, Nfreq |
There was a problem hiding this comment.
It looks like "pol" isn't used. I don't remember what the interp_data axes are offhand, but you should be careful that the correct polarization is being returned.
Part of the complication with supporting E-field beams it that you have to make sure you're extracting the right feed pair. I suppose the way to do this is (as with the existing code) only support on-diagonal components (XX and YY), and then the "beam_pol" option should be either X or Y.
There was a problem hiding this comment.
I'll check over this. I haven't been quite sure about the handling of polarization in the sim packages - if they're all doing the same thing or meant to be. Phil is adding polarization to hera_sim so he's grappling with this as well.
This pull request contains a few minor changes and one significant change. All the changes are backwards compatible. I’ve run all the tests in the tests/ directory and they all pass. I’ve added another test script.
The minor changes are:
The significant change is to allow the use of external beams derived from and including pyuvsim AnalyticBeam. We (Phil Bull’s group) want to use the Fagnoni-type beams that have been developed for hera_sim: https://github.com/HERA-Team/hera_sim/blob/polybeam/hera_sim/beams.py. The beams do not need to be the same for all antennas, and are passed as a list to set_beam(). UVBeam objects could also be used, but UVBeam has a lot of other parameters which I have not implemented, and also are not implemented by pyuvsim AnalyticBeam. All these beams probably need a consistent interface.
There is a slight issue with this enhancement because the beams defined within healvis (healvis AnalyticBeam) are baseline beams, but the external beams are antenna beams. Antenna beams for a baseline must be multiplied when generating visibilities. Code has been added to keep track of per-antenna beams and then multiply beams when visibilities are calculated in _vis_calc().