Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper tracking of flagged times #328

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Proper tracking of flagged times #328

wants to merge 1 commit into from

Conversation

nicholebarry
Copy link
Contributor

This fix addresses issue #321, where the auto calibration was using times which should have been flagged.

This underlying root cause is that the time_use array was not checked against the weights array. In the case of SSINS, the weights for an entire time step will flagged, but this was not propagating to the time_use array in FHD. The time_use array is used a few circumstances, the two major instances being 1) auto calibration averaging and 2) noise calculation. Given that this affects the results in a few places, there is an overall change in the PS.

Below is a test against a control for an observation that had 4 SSINS flagged times. Differences are small and somewhat hard to interpret. Not sure if we want to pursue this differences deeper or if we are happy to take the fact that this is the proper time_use handling.

fhd_nb_ntime_noimgclip_dft_averemove_swbh_dencorr__control2_1088281088_minus_test2_1088281088_2dkpower
fhd_nb_ntime_noimgclip_dft_averemove_swbh_dencorr__control2_1088281088_diffratio_test2_1088281088_2dkpower

Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for hunting this down!! I'm not worried about the PS changes, they look small and reasonable and this is clearly the more correct thing to do. I have one comment about the polarization aspect that doesn't need to block this but I want to understand a little better.

Comment on lines +130 to +131
; If any polarization is fully flagged, then count that time as flagged
time_use[ti] *= total((*vis_weight_ptr[pol_i])[*, bin_offset[ti]:bin_offset[ti+1]-1] > 0) GT 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to be revisited in the case that Dara developed for HERA where antenna flagging can be per pol. I don't think we want to support different per-time flagging per pol if both pols are included, but we do want to support entirely flagging one pol of an antenna and not flagging the other pol. We can push that to another PR if we want just wanted to note the potential conflict.

@bhazelton
Copy link
Member

@miguelfmorales you should see this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants