-
Notifications
You must be signed in to change notification settings - Fork 87
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
Conversation
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.
Hi Mark, Secondly, of course it would be good to have some kind of unit test for |
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 |
Mmh. Probably best to discuss the location of the table descriptions with Tammo. And yes, I remember now that we did all the testing in the CASA unit tests. |
ms/MSOper/MSConcat.cc
Outdated
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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:
- 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. - some rudimentary unit test within casacore
@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. |
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. |
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. |
A separate note is fine as well, just as long as we have the description available, specially now that MSv4 is being discussed. |
See issue #1307