ratt-ru / QuartiCal

CubiCal, but with greater power.
MIT License
7 stars 4 forks source link

Change request: Remove interpolated terms from iter recipe #250

Closed bennahugo closed 10 months ago

bennahugo commented 1 year ago

The current specification of recipes with iteration counts intermixed with applied terms is very confusing when you start iterating multiple times - it looks like a loaded term can be solved further because transferring amplitude and bandpasses from a primary and solving for phases resulted in the flux scales being changed - the ball shifts amplitude!

input_ms.path=s0_offsets.flagged.avg.uncorrected.ms \
input_ms.data_column=DATA \
input_ms.time_chunk=100000000s \
input_ms.select_fields="[0]" \
input_ms.group_by="[FIELD_ID,DATA_DESC_ID]" \
input_model.recipe=MODEL_DATA \
solver.terms="['Ga','B','Gp2']" \
solver.iter_recipe="[0,0,20,20]" \
output.products="[corrected_data]" \
output.columns="['CORRECTED_DATA']" \
output.overwrite=False \
output.gain_directory=gains.qc.secphase \
Ga.type=amplitude \
Ga.time_interval=64s \
Ga.freq_interval=0 \
Ga.load_from=gains.qc/Ga \
B.type=diag_complex \
B.time_interval=1000000000s \
B.freq_interval=1 \
B.load_from=gains.qc/B \
Gp2.type=phase \
Gp2.time_interval=64s \
Gp2.freq_interval=0

I really think the recipe should consists of terms being arranged to apply and terms arranged to be solved. Ie similar to the CASA interface where caltable is specified and the gaintable list sets the terms in the chain fixed and loaded from file.

bennahugo commented 1 year ago

I also think gain types in the apply setting should be inferrable from the table data - similar to CASA - you should only need to give a list of caltable files.

JSKenyon commented 1 year ago

Unfortunately, this is just user error. All terms in the chain are treated equally, regardless of whether they are loaded or computed. The reason why I am unwilling to change this (at present) as it forces you into a regime whether a term is either loaded or solved. QC has the flexibility to do additional refinements to already solved terms and I intend to retain that functionality. In the above example, solver.iter_recipe="[0,0,20,20]" would have resulted in 0 iterations on Ga, 0 iterations on B, 20 iterations on Gp2 and 20 iterations on Ga. If one of those changed unexpectedly, it must have had a non-zero iteration count associated with it. Note that if you load a term with a solution interval which is different that the one you solved with, QC will interpolate it to the required resolution.

As to your second point, yes, it is possible to get the type from the loaded term. I am working on this at present along with a number of other improvements to the interpolation code. It is a very fiddly problem though so it is taking me some time to come up with a neat/robust approach.

bennahugo commented 1 year ago

Hmm. It is unclear to me what happens with a loaded K term where 0 and lag initialize is set to true for instance - the behaviour for loaded and solve is identical in that meaning.

I think there should be an additional flag per gain term called 'fixed' that stops further adjustment of loaded terms.

On Thu, 04 May 2023, 09:12 JSKenyon, @.***> wrote:

Unfortunately, this is just user error. All terms in the chain are treated equally, regardless of whether they are loaded or computed. The reason why I am unwilling to change this (at present) as it forces you into a regime whether a term is either loaded or solved. QC has the flexibility to do additional refinements to already solved terms and I intend to retain that functionality. In the above example, solver.iter_recipe="[0,0,20,20]" would have resulted in 0 iterations on Ga, 0 iterations on B, 20 iterations on Gp2 and 20 iterations on Ga. If one of those changed unexpectedly, it must have had a non-zero iteration count associated with it. Note that if you load a term with a solution interval which is different that the one you solved with, QC will interpolate it to the required resolution.

As to your second point, yes, it is possible to get the type from the loaded term. I am working on this at present along with a number of other improvements to the interpolation code. It is a very fiddly problem though so it is taking me some time to come up with a neat/robust approach.

— Reply to this email directly, view it on GitHub https://github.com/ratt-ru/QuartiCal/issues/250#issuecomment-1534200814, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6U6T2G2GY56X2O4Q6TXENJGLANCNFSM6AAAAAAXU5DZQQ . You are receiving this because you authored the thread.Message ID: @.***>

JSKenyon commented 1 year ago

I am also going to flip the default for the things like the delay init, for similar reasons. But yeah, maybe there is room for an explicit option that stops adjustments.

JSKenyon commented 1 year ago

Note that initial estimates are never computed on a loaded/interpolated term, so that shouldn't trip anyone up.

bennahugo commented 10 months ago

Going to close this one - it may be nice to have an applycal step to wrap quartical for the apply only case, but I leave this for a later exercise.