-
Notifications
You must be signed in to change notification settings - Fork 3
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
When should we compute values w.r.t. TIME and TIME_CENTROID respectively? #249
Comments
In theory, using TIME_CENTROID is the most precise way. In practice, most applications will not see a difference in the result (for things like PA and DIEs, it's certainly down in the noise and/or machine precision). So I would think really hard before we (a) pour Simon time into this, (b) give up any significant performance. Arguing the other side: at some point we need to support BDA datasets, in which case many things become per-baseline anyway... |
I completely agree with OMS on this issue. |
@o-smirnov @landmanbester @SaiyanPrince @JSKenyon @MichelleLochner @bennahugo @rdeane @twillis449. I've written up a short latex document describing the accuracy (DDFacet, Cubical, Bayesian Inference) vs performance (Bayesian Inference) issues facing Montblanc. I'd appreciate your input on this because it seems like we're going to have to sacrifice some performance in order to obtain accuracy when data is flagged. |
/cc @JoshVStaden @IanHeywood too |
Updated document to indicate that TIME_CENTROID usually differs from TIME when data has been averaged... |
Two comments
|
Seems like per-baseline complex phase (from UVW coordinates) is going to be the way forward for the purposes of correctness. However, I still aim to support the antenna decomposition so I've been putting switches into the tensorflow kernels in the dask branch to allow plugging in terms in various places. |
The Measurement Set specification says that UVW coordinates are calculated w.r.t TIME_CENTROID.
This raises questions as to whether we should calculate other quantities w.r.t. TIME_CENTROID. I can think of:
Related to #248
/cc @JSKenyon @o-smirnov @landmanbester
The text was updated successfully, but these errors were encountered: