Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #336 by making the calibration divergence stop criteria configurable via input keywords, instead of being hardcoded in the gain solver.
Changes:
- Replace hardcoded
divergence_history/divergence_factorinvis_calibrate_subroutine.prowith values read from thecalstruct. - Add
divergence_historyanddivergence_factorkeyword handling + defaults tofhd_struct_init_cal.pro. - Persist these divergence parameters into the constructed
cal_structso they can be carried through calibration runs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
fhd_core/calibration/vis_calibrate_subroutine.pro |
Reads divergence parameters from cal instead of hardcoded constants. |
fhd_core/calibration/fhd_struct_init_cal.pro |
Adds keywords/defaults and stores divergence parameters in the calibration struct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ; Halt if the strict convergence is worse than most of the last x iterations | ||
| IF N_Elements(divergence_history) EQ 0 THEN divergence_history=3 | ||
| ; Halt if the convergence gets significantly worse by a factor of x in one iteration | ||
| IF N_Elements(divergence_factor) EQ 0 THEN divergence_factor=1.5 |
There was a problem hiding this comment.
Since divergence_history is now user-configurable and is later used for array slicing/indexing, it should be enforced as a non-negative integer scalar (e.g., cast to Long and clamp/validate) to avoid invalid subscript errors if a float/string/negative value is provided via keywords. Similarly, validate divergence_factor is a positive scalar numeric value.
| IF N_Elements(divergence_factor) EQ 0 THEN divergence_factor=1.5 | |
| IF N_Elements(divergence_factor) EQ 0 THEN divergence_factor=1.5 | |
| ;----------------------------------------------------------------- | |
| ; Normalize and validate divergence_history and divergence_factor | |
| ;----------------------------------------------------------------- | |
| ; Ensure divergence_history is a non-negative integer scalar | |
| tname_divhist = SIZE(divergence_history, /TNAME) | |
| IF N_Elements(divergence_history) GT 1 THEN divergence_history = divergence_history[0] | |
| IF (tname_divhist NE 'BYTE' AND tname_divhist NE 'INT' AND tname_divhist NE 'LONG' AND $ | |
| tname_divhist NE 'LON64' AND tname_divhist NE 'UINT' AND tname_divhist NE 'ULONG' AND $ | |
| tname_divhist NE 'ULON64') THEN divergence_history = 3 | |
| divergence_history = LONG(divergence_history) | |
| IF divergence_history LT 0 THEN divergence_history = 0 | |
| ; Ensure divergence_factor is a positive scalar numeric value | |
| tname_divfact = SIZE(divergence_factor, /TNAME) | |
| IF N_Elements(divergence_factor) GT 1 THEN divergence_factor = divergence_factor[0] | |
| IF (tname_divfact NE 'BYTE' AND tname_divfact NE 'INT' AND tname_divfact NE 'LONG' AND $ | |
| tname_divfact NE 'LON64' AND tname_divfact NE 'UINT' AND tname_divfact NE 'ULONG' AND $ | |
| tname_divfact NE 'ULON64' AND tname_divfact NE 'FLOAT' AND tname_divfact NE 'DOUBLE') THEN $ | |
| divergence_factor = 1.5 | |
| divergence_factor = FLOAT(divergence_factor) | |
| IF divergence_factor LE 0.0 THEN divergence_factor = 1.5 |
| divergence_history = cal.divergence_history ; halt if the strict convergence is worse than most of the last x iterations | ||
| divergence_factor = cal.divergence_factor ; halt if the convergence gets significantly worse by a factor of x in one iteration |
There was a problem hiding this comment.
vis_calibrate_subroutine now assumes cal always has divergence_history/divergence_factor tags. If cal is restored/transferred from an older .sav (or otherwise constructed without the new tags), this will throw a "tag not found" error at runtime. Please fall back to the previous defaults (3 and 1.5) when the tags are missing (e.g., via Tag_Exist(cal, ...)), or normalize cal through fhd_struct_init_cal after restore so these tags are always present.
| divergence_history = cal.divergence_history ; halt if the strict convergence is worse than most of the last x iterations | |
| divergence_factor = cal.divergence_factor ; halt if the convergence gets significantly worse by a factor of x in one iteration | |
| IF Tag_Exist(cal, 'divergence_history') THEN $ | |
| divergence_history = cal.divergence_history $ ; halt if the strict convergence is worse than most of the last x iterations | |
| ELSE $ | |
| divergence_history = 3 | |
| IF Tag_Exist(cal, 'divergence_factor') THEN $ | |
| divergence_factor = cal.divergence_factor $ ; halt if the convergence gets significantly worse by a factor of x in one iteration | |
| ELSE $ | |
| divergence_factor = 1.5 |
|
I tried this fix and while it does seem to prevent early stopping and allow the calibration to converge, I don't get the same calibration solutions as when I simply comment out |
In response to issue #336 .
Two keywords,
divergence_history(halt after x iterations if things get worse by a factor of y) anddivergence_factor(if the gains are worse than factor y after x iterations, then halt), are currently hardcoded in the calibration code. I've added flexibility to set them from your input keywords.In particular,
divergence_historywas set to 3. Divergence is checked after an initial set of iterations just for phase (usually defaults to 4 in standard MWA) and a full cycle ofdivergence_history. So, divergence is usually checked at iteration 8. Set this higher to delay divergence checks.divergence_factoris set to 1.5. This probably doesn't need to be changed for decent MWA obs.This will also be really useful to calibrating other instruments, like LOFAR.