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

Dcp uvfits rdate fix #1436

Conversation

telegraphic
Copy link
Contributor

Fixes to RDATE and DATE-OBS header parameters in UVFITS writer

Description

As per #1433, RDATE calculation is odd. I've also updated DATE-OBS to be YYYY-MM-DD, instead of ISOT, as this is what AIPS memo 117 uses.

Motivation and Context

This fixes incorrect RDATE and DATE-OBS header parameters. These parameters are used by MIRIAD when importing UVFITS files, and MIRIAD was getting confused. I'm not sure if CASA uses these when importing uvfits (if it did, I think this would have been caught sooner?)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change (documentation changes only)
  • Version change
  • Build or continuous integration change

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.

Bugfix notes

One unit test now fails, test_uvfits_extra_params, due to DUT1 being slightly different (-0.2136028 vs expected -0.2137079). Not sure if test is incorrect, or astropy's DUT1 calculation is unexpected.

@telegraphic
Copy link
Contributor Author

Note: I'm not going to attempt to fix linting issues as I'm clearly cursed 👻 👕

@kartographer
Copy link
Contributor

@telegraphic -- since there's a little bit of clean-up to do here (both w/ the failing test and the linting), do you mind if I pull this over into a new branch so that I can clean a few things up? That should allow us to get this in quickly.

@telegraphic telegraphic mentioned this pull request May 24, 2024
11 tasks
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.

None yet

3 participants