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

Add support for Earth Orientation Parameters #1318

Merged
merged 2 commits into from
Nov 24, 2023
Merged

Conversation

kettenis
Copy link
Contributor

See issue #1307

Import Earth Orientation Parameters (EOPs) from FITS-IDI files and
convert them into an EAUTH_ORIENTATION subtable as documented in
issue casacore#1307.  These EOPs are necessary for applying corrections
in case more accurate values become available after correlation.
This is import for astrometric VLBI observations.
@dpetry dpetry self-assigned this Sep 29, 2023
@dpetry
Copy link
Contributor

dpetry commented Sep 29, 2023

Hi Mark,
so, this PR is adding a whole new optional sub-table to the MS!
As far as I can see, this new subtable is only documented in casacore issue #1307.
I wonder if at least a basic description of the table, i.e. one sentence about its purpose
plus the column descriptions, could also go into the code?
Also pointing out the "key" columns for accessing the table.

Secondly, of course it would be good to have some kind of unit test for
writing and reading this table.

@kettenis
Copy link
Contributor Author

Hi Dirk,

Yes, I've followed the approach taken with the GAIN_CURVE and PHASE_CAL tables, which also have the table documented in issues. Maybe it it is a good idea to add those descriptions in somewhere in the repository itself. The issues use markdown, so copying the important bits out of the issue into a .md file is trivial. And github can render those properly. What would be an appropriate place for those files?

As for unit tests, there will be tests in CASA of course, and so far we've not added any FITS-IDI tests in casacore. If we want that, we probably need two or three FITS-IDI files in the data repository that casacore uses for its unit tests.

Cheers,

Mark

@dpetry
Copy link
Contributor

dpetry commented Oct 2, 2023

Mmh. Probably best to discuss the location of the table descriptions with Tammo.
Not sure if there are other cases of precedence.
They are in principle extensions of the MS specification. Those are not done very often ...

And yes, I remember now that we did all the testing in the CASA unit tests.
You are not modifying the ms/MeasurementSets part of casacore to introduce these
new optional tables but msfits/MSFits .
And the code in msfits/MSFits/test/ does not exercise any FITSIDI-related routines.
Maybe it is time to change this with at least one test "tMSFITSIDIInput.cc" and one corresponding FITSIDI input dataset
which triggers the creation of as many of the optional subtables as possible?

Bool otherEOPNull = (!otherMS.keywordSet().isDefined("EARTH_ORIENTATION") || (otherMS.keywordSet().asTable("EARTH_ORIENTATION").nrow() == 0));

if(itsEOPNull && otherEOPNull){ // neither of the two MSs do have valid EOP tables
os << LogIO::NORMAL << "No valid EOP tables present. Result won't have one either." << LogIO::POST;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be done with LogIO::DEBUG since this is an optional table and we don't need to record that it is not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense; used LogIO::DEBUG1 since LogIO::DEBUG doesn't exist and LogIO::DEBUG1 is what is used elsewhere in this file.

Adjusting OBSERVATION_IDs if necessary.
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.

OK, I don't want to hold up your development.
Clearly there are two nice-to-haves which you should consider adding at some point:

  1. ca. half an A4 page worth of text describing the VLBI-related add-ons to the MS and
    from where they are typically filled, i.e. some kind of FITSIDI->MS mapping.
    Just the table names on both sides would already be helpful.
    Leaving this in some issue text is not good. These things get lost or at least are
    very difficult to find after a few years.
  2. some rudimentary unit test within casacore

@kettenis
Copy link
Contributor Author

@tammojan Can this PR be merged?

Regarding documenting the VLBI extension tables, what would the best place to document those? Easiest would be if I could add a markdown file somewhere in the tree that documents them. Or perhaps a separate markdown file for each. The documentation in the issues is already markdown and github knows how to render them.

@tammojan
Copy link
Contributor

As discussed offline with @kettenis, I think the 'traditional' way to store casacore notes is on https://github.com/casacore/casacore-notes . Technically a markdown in this repository would also be good, but this addition is so small that I'd rather not change the way of working because of this. So @kettenis will (shall/should/may) add a note to casacore-notes.

@tammojan
Copy link
Contributor

In fact, @kettenis you could make a pull request against casacore note 264 (the MSv3 draft).

@tammojan tammojan merged commit 2cbdefe into casacore:master Nov 24, 2023
6 of 7 checks passed
@kettenis
Copy link
Contributor Author

In fact, @kettenis you could make a pull request against casacore note 264 (the MSv3 draft).

@tammojan I think it makes sense to have it as a separate note. These are de-facto MSv2 extensions and I'm not sure to what extent the MSv3 draft is relevant now that MSv4 is being discussed... What I could do is have the actual table definitions in a separate .tex file that gets included in both the separate note and the MSv3 draft note.

@tammojan
Copy link
Contributor

A separate note is fine as well, just as long as we have the description available, specially now that MSv4 is being discussed.

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

3 participants