jjtobin / auto_selfcal

MIT License
17 stars 8 forks source link

SPWs can vary from EB to EB, causing problems #12

Closed psheehan closed 1 year ago

psheehan commented 1 year ago

In testing mosaics today I came across a dataset with multiple EBs for which one of the EBs had the SPWs slightly shifted in terms of SPW ID. This caused the code to break on mstransform as it was asking fow SPWs that did not exist, though it is possible it would have broken elsewhere as well.

I started putting together some fixes, essentially better tracking the SPWs on a per-EB basis to that each EB only ever asks for the SPWs associated with it, and that lives in a new branch here: https://github.com/psheehan/auto_selfcal/tree/spws_vary_across_EBs, with the changes happening in this commit: https://github.com/psheehan/auto_selfcal/commit/a1102b5be8cceeb8534dddfab4401945ceab0282. I've thus far tested and it should work through splitting to the selfcal MS files. When the changes I've made touched pieces of code further downstream I've also made those fixes, but haven't tested that those work, nor have I yet checked that there aren't any other places that things don't work correctly. I've started a full run on the dataset this afternoon and will be able to check more carefully for any further fixes that might be needed next week. @jjtobin, since you have put much more thought into spw tracking thus far, it'd probably be good for you to look at this carefully to make sure I haven't missed anything important.

In case you want to take a look, the dataset lives in my selfcal-prototyping directory under 2016.1.00053.S.

Also, though I've built the fixes on top of the development branch, if we wanted to put this into the main branch sooner, most of the changes happen in selfcal_helpers so I think it wouldn't be hard to cherry-pick it over there, or to recreate in the main branch.

jjtobin commented 1 year ago

I think your changes look quite reasonable. When reading this I was thinking that I had done most of this already, but what I had implemented only worked if the spw numbers were all the same in each EB, while the frequencies for each number could be different. This does the next logical step of not assuming that the numbers are all the same.

psheehan commented 1 year ago

Ok great - the run is done and I'm going to check it over today... but do you think there are any additional places further downstream where modifications might be necessary? Or should the spw information propagate properly with these changes?

jjtobin commented 1 year ago

Offhand I don't know if it needs to propagate further, but that could be revealed in further tests, but the fix is probably safe to apply to main and we can fix things that come up later.

You might check my commits to do the spectral scan support to get an idea of other places where the information might need to propagate.

psheehan commented 1 year ago

Ok, unfortunately the test dataset that I was looking at didn't do any selfcal because the estimated SNR was too low. I'm going to try to turn that down to try to force it to at least try - I don't expect it to be successful but at least it'll will go through the motions.

Looking through your changes, the two other places I see potential problems are flagging from cont.dat, as it seems like cont.dat doesn't separately identify lines in those SPWs in any way so I might formally need a mapping from SPWs in one EB to another, and analyzing the inf_EB fallback, not that I expect problems, but see changes there and haven't tried it out to see if it works.

I'll try to look at the cont.dat aspect more closely over the next few days. If I can force it to do selfcal, I can also check the latter

psheehan commented 1 year ago

Closing because this was fixed across a few commits on the development branch:

https://github.com/psheehan/auto_selfcal/commit/a1102b5be8cceeb8534dddfab4401945ceab0282 https://github.com/psheehan/auto_selfcal/commit/c01277957518e9306b29d3702d808e764e436ac7 https://github.com/psheehan/auto_selfcal/commit/36404a45ef21ce6fc75d4832d2541313d90c5570 https://github.com/psheehan/auto_selfcal/commit/7d046e9de41a6ea06402f3b64a89a026eb8cb95b https://github.com/psheehan/auto_selfcal/commit/c252ac62b34e6de153ab8b634c850dd4d14d35f5