Add ability to perform simulations with per-antenna beams#63
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
+ Coverage 93.18% 95.22% +2.04%
==========================================
Files 18 18
Lines 484 566 +82
Branches 64 86 +22
==========================================
+ Hits 451 539 +88
+ Misses 26 17 -9
- Partials 7 10 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds per-antenna beam support to fftvis.simulate_vis (via a list of beams plus beam_idx) and propagates that capability through the CPU simulation engine, along with new tests covering multi-beam behavior and beam-pair flux helpers.
Changes:
- Extend the public
simulate_viswrapper to accept a single beam or a list of beams and addbeam_idx. - Update the CPU simulation engine to route baselines by beam-pair and evaluate apparent flux for mixed-beam baselines.
- Add/expand tests for multi-beam simulations and new beam-evaluation helper paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/fftvis/wrapper.py |
Adds beam_idx, nchunks, source_buffer, and converts beam input into an internal beam_list. |
src/fftvis/cpu/cpu_simulate.py |
Threads beam_list/beam_idx through chunk evaluation and implements beam-pair routing logic. |
src/fftvis/cpu/beams.py |
Adds prepare_beam_evaluation plus cross-beam polarized apparent-flux kernels. |
tests/test_wrapper.py |
Adds wrapper-level validation tests for beam list / beam_idx error cases. |
tests/test_cpu_simulate.py |
Adds an fftvis-vs-matvis multi-beam regression test and updates calls for new arguments. |
tests/test_cpu_beams.py |
Adds unit tests for beam-pair routing and new polarized apparent-flux helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @tyler-a-cox -- I'm going to let you deal with Copilot's suggestions first, and then please ping me and I'll take a final look. From my side, the code is working on bridges, so there should be nothing major to fix. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…aluation Agent-Logs-Url: https://github.com/tyler-a-cox/fftvis/sessions/53224b26-52a0-4803-be07-4c1c70648550 Co-authored-by: tyler-a-cox <17678594+tyler-a-cox@users.noreply.github.com>
Agent-Logs-Url: https://github.com/tyler-a-cox/fftvis/sessions/becb649d-88b1-4811-be20-a764b9d4db52 Co-authored-by: tyler-a-cox <17678594+tyler-a-cox@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@steven-murray okay, this should be good for you to review now |
steven-murray
left a comment
There was a problem hiding this comment.
Thanks @tyler-a-cox., this is looking great. Thanks also for adding the option to specify the max memory. I thnk the logic that copilot added might be wrong, so maybe have another look at that, to make sure. Otherwise this is good to go.
| # Get number of beams: use max index + 1 so non-contiguous indices | ||
| # (e.g. [0, 2, 2]) still produce the correct set of beam pairs. | ||
| nbeams = int(np.max(beam_idx)) + 1 |
There was a problem hiding this comment.
Hmmm, this actually doesn't seem right to me. We should only include beam indices that actually exist, right? We don't need to use all pairs in the range from (0,maxidx)?
There was a problem hiding this comment.
I'm now building the unique beam pairs array with pairs actually used in beam_idx array
| if isinstance(beam, UVBeam): | ||
| required_shm += beam.data_array.nbytes | ||
| elif isinstance(beam, BeamInterface) and getattr(beam, "_isuvbeam", False): | ||
| required_shm += beam.beam.data_array.nbytes |
There was a problem hiding this comment.
Probably the right thing to do here actually is to only ever deal with the BeamInterface (so, if UVBeam is passed to the wrapper, convert it to a BeamInterface there, so the logic throughout the rest of the code is more uniform).
There was a problem hiding this comment.
Went back to just using BeamInterface
steven-murray
left a comment
There was a problem hiding this comment.
Looks correct to me!
This PR adds the functionality to allow users provide per-antenna beams and beam_idx as
matvisdoes. There is a slight difference between the keyword argument used to pass beams between matvis and fftvis (i.ebeamin fftvis vs.beamsin matvis). It may be a good idea for us to change that in the future, but for now, I left the argument asbeamkeep this from breaking the API.