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

Warn about old VLBA data that needs FFT artifact correction #1319

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

kettenis
Copy link
Contributor

No description provided.

@kettenis
Copy link
Contributor Author

Justin Linford from the VLBA told me is happy with the way this would warn (their) users about data that may lack certain corrections when imported into CASA. So I'm removing draft status from this PR.

@kettenis kettenis marked this pull request as ready for review September 29, 2023 11:45
@kettenis
Copy link
Contributor Author

@dpetry Any comments on this one? I'll probably have to rebase this PR after #1318 gets merged, but that shouldn't be a big issue.

@dpetry dpetry self-assigned this Feb 28, 2024
@dpetry
Copy link
Contributor

dpetry commented Feb 28, 2024

Sorry, didn't see this before. Will look at it in detail tomorrow.

@dpetry
Copy link
Contributor

dpetry commented Feb 28, 2024

@kettenis , hi, I had a first look. I don't think all warnings should be reduced to normal infos.
I think those messages which are about deprecated or incorrectly used FITS-IDI features and similar, should remain
warnings or at least have some string in them which people can grep for.
But I'll give a complete review tomorrow ..

Copy link
Contributor

@dpetry dpetry left a comment

Choose a reason for hiding this comment

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

I agree that all the "treating as scalar" warnings can be turned to INFO.
Also the Double should be Float can be INFO.
But the "not yet implemented" and "will ignore it" messages should stay
warnings such that people realise that some of the input is not making it
into the MS.

@kettenis
Copy link
Contributor Author

kettenis commented Feb 29, 2024

@dpetry thanks for the review. Yes maybe I went a little bit too far in demoting the warnings got standard FITS-IDI tables that we don't handle yet. For the BASELINE, BANDPASS and CALIBRATION tables that is not an issue, since those tables are not present in typical archival data. I'm not even sure if code exists to produce these. For the INTERFEROMETER_MODEL your concern is certainly justified. Even though there is no code in CASA to use the correlator model, there is general consensus that this data should be added to the MS. A new table was proposed as part of MSv3 and is still on the table for MSv4.

I've updated the pull request and also rebased it against the current master.

Copy link
Contributor

@dpetry dpetry left a comment

Choose a reason for hiding this comment

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

@kettenis , this looks good except for the very last message about the unconventional extension.
IMHO that should remain a warning since people should be told if their input
contains something which cannot be interpreted and is ignored.
(Sorry if I'm delaying you.)

@dpetry
Copy link
Contributor

dpetry commented Mar 1, 2024

@kettenis , hi Mark, did you see my comment above? Please just turn that last message back into a warning and I approve.

@kettenis
Copy link
Contributor Author

kettenis commented Mar 4, 2024

@dpetry
Hi Dirk,

Unfortunately the VLBA data tends to have a few non-standard tables attached to it. Some of those are empty (no rows) or really are of no interest to the modern user (TAPE_STATISTICS). And these tables tend to come at the end so the warning you're talking about tends to come at the end of the output the user sees. The VLBA stakeholders are worried that this means users will miss the warnings.

How about printing that warning only if the number of rows in the table is greater than zero and exclude the TAPE_STATISTICS table?

      if (extname != "TAPE_STATISTICS" && nrows() > 0) {
        *itsLog << LogIO::WARN << "Extension " << extname
                << " not part of the FITS-IDI convention. Will ignore it." << LogIO::POST;
      }

@dpetry
Copy link
Contributor

dpetry commented Mar 4, 2024

@kettenis , yes, that's a great idea. Please go ahead with this.

Warning about deviations from the FITS-IDI convention that are
harmless is somewhat counterproductive as this increases the risk
that users miss warnings that really do matter.  So demote these
to normal messages.  Also don't warn about unhandled tables that
are empty or TAPE_STATISTICS tables.
@kettenis
Copy link
Contributor Author

kettenis commented Mar 4, 2024

@dpetry Thanks, done!

Copy link
Contributor

@dpetry dpetry left a comment

Choose a reason for hiding this comment

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

Approved.

@dpetry dpetry merged commit c6c5513 into casacore:master Mar 5, 2024
12 of 13 checks passed
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.

None yet

2 participants