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

Better handing of discrepancies for LSTs and telescope position in check #1356

Merged
merged 7 commits into from
Nov 6, 2023

Conversation

kartographer
Copy link
Contributor

Description

Updates handling of lst_array and telescope_position in terms of when warnings and errors get issued inside of check for UVData, UVCal, and UVFlag. More specifically, the tolerance for when the warning about "LSTs not set correctly" has been increased to 15 milliarcsec, to better reflect the typical uncertainty in these values at the time of observation (due to the ever-changing DUT1 ). Additionally, check now looks at the combination of telescope location and antenna positions in determining whether or not a given set of positions are "reasonable", and when a discrepancy is discovered, a warning is now issued instead of an error.

Motivation and Context

Relevant issues below, but in both cases, the prior behavior in check was resulting in undesired behavior. In the case of the LSTs, a lot of spurious warnings were being generated that were confusing/concerning some users in situations where the "issues" were totally benign. In the case of the telescope position handing, it was breaking support for VLBA/VLBI-generated UVFITS file (without disabling parts of check), whose telescope location (phase reference center) is not neccessarily on the surface of the Earth.

Closes #1348
Closes #1352

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

Bug fix checklist:

  • My fix includes a new test that breaks as a result of the bug (if possible).
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #1356 (0fe6159) into main (d63b9a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1356   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          36       36           
  Lines       20185    20202   +17     
=======================================
+ Hits        20169    20186   +17     
  Misses         16       16           
Files Coverage Δ
pyuvdata/parameter.py 100.00% <ø> (ø)
pyuvdata/utils.py 100.00% <100.00%> (ø)
pyuvdata/uvcal/uvcal.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/miriad.py 98.47% <100.00%> (ø)
pyuvdata/uvdata/uvdata.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/uvfits.py 100.00% <ø> (ø)
pyuvdata/uvdata/uvh5.py 100.00% <ø> (ø)
pyuvdata/uvflag/uvflag.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d63b9a5...0fe6159. Read the comment docs.

@kartographer
Copy link
Contributor Author

One related item -- a small file from VLBA (274 kb) outside of the repo was used to check for VLBA support, which could in principle be added to the repository. Not necessarily something for this PR, but something that was stumbled upon over the course of developing it.

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.

The changes looks great! There's just a little problem with the change log.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kartographer
Copy link
Contributor Author

I made one additional adjustment here, which was to increase the LST tolerance to 5 ms (75 mas in angle). This is because the estimated uncertainty in DUT1 at one week from IERS Bulletin A is about 1 ms (which are published weekly), and so I set the "detection limit" here at 5 sigma to prevent spurious warnings. I think this should be sufficient for nearly all applications, but should still detect gross errors (> 1 sec) quite easily.

pyuvdata/utils.py Outdated Show resolved Hide resolved
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.

Looks good, thanks!

@bhazelton bhazelton merged commit b75d028 into main Nov 6, 2023
44 checks passed
@bhazelton bhazelton deleted the better_tols branch November 6, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle telescope locations at the center of the earth for VLBI LST stringency issues
2 participants