Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses critical issues with injected transient positioning in the imaging pipeline. The main problem was a double sign flip that placed injected transients diametrically opposite to their intended positions. Additionally, the code was incorrectly using the input radec instead of the rephased radec_new as the reference direction, causing transients to appear to move in on-the-fly (OTF) imaging scenarios.
Key changes:
- Fixed transient injection to use rephased coordinates (
radec_new) instead of original coordinates (radec) - Corrected sign conventions for transient l,m coordinates (now negated to match wgridder conventions)
- Added protection against invalid wgridder padding values in hci.py
- Improved CI workflow reliability with FTP connection flags
- Code cleanup removing commented-out dead code
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pfb/utils/stokes2im.py | Core fix for transient positioning: uses radec_new for reference direction and applies correct sign flips to l,m coordinates; removes commented dead code |
| pfb/utils/astrometry.py | Fixes uvw_rotate to avoid in-place modification by creating a copy |
| pfb/workers/hci.py | Adds validation to cap min_padding at 2.5 to prevent wgridder errors |
| .gitignore | Excludes test data directory from version control |
| .github/workflows/ci.yml | Adds --disable-epsv flag to curl commands for more reliable FTP downloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pfb/utils/astrometry.py
Outdated
| uvw_new[0] = mat_11 * uvw[0] + mat_12 * uvw[1] + mat_13 * uvw[3] | ||
| uvw_new[1] = mat_21 * uvw[0] + mat_22 * uvw[1] + mat_23 * uvw[3] | ||
| uvw_new[2] = mat_31 * uvw[0] + mat_32 * uvw[1] + mat_33 * uvw[3] |
There was a problem hiding this comment.
The matrix multiplication uses uvw[3] which is out of bounds. UVW coordinates are 3-dimensional (u, v, w at indices 0, 1, 2), so the third column index should be uvw[2] not uvw[3]. This would cause an IndexError when the function is called.
| uvw_new[0] = mat_11 * uvw[0] + mat_12 * uvw[1] + mat_13 * uvw[3] | |
| uvw_new[1] = mat_21 * uvw[0] + mat_22 * uvw[1] + mat_23 * uvw[3] | |
| uvw_new[2] = mat_31 * uvw[0] + mat_32 * uvw[1] + mat_33 * uvw[3] | |
| uvw_new[0] = mat_11 * uvw[0] + mat_12 * uvw[1] + mat_13 * uvw[2] | |
| uvw_new[1] = mat_21 * uvw[0] + mat_22 * uvw[1] + mat_23 * uvw[2] | |
| uvw_new[2] = mat_31 * uvw[0] + mat_32 * uvw[1] + mat_33 * uvw[2] |
| signu*uvw[:, 0:1]*x0t*signx + | ||
| signv*uvw[:, 1:2]*y0t*signy - |
There was a problem hiding this comment.
The comment says "do not apply signx and signy as that is only flipped inside the wgridder", but the code on lines 432-433 still multiplies by signx and signy. This creates confusion about whether the signs should be applied or not. Either the comment or the implementation needs to be corrected to match the intended behavior.
| signu*uvw[:, 0:1]*x0t*signx + | |
| signv*uvw[:, 1:2]*y0t*signy - | |
| signu*uvw[:, 0:1]*x0t + | |
| signv*uvw[:, 1:2]*y0t - |
|
Thanks for the update Landman |
2 participants
Previously there was a double sign flip so injected transients would have been diametrically opposite where they needed to be relative to the phase center. I was also using the input radec instead of the rephased radec as the reference direction when computing lm for the injected transients so they would have appeared to move for the OTF case. This all seems fixed now. The missing feature is the beam application to injected transients. Still figuring out how to do this neatly. Pinging @TariqBlecher @Victoria-Samboco @SihlanguI