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
DM-18864: Now support UNDEF in FITS headers #446
Conversation
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.
Very nice.
return metadata | ||
|
||
def testSimpleIO(self): | ||
|
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.
Blank line looks weird.
PropertySet (and List) can handle arrays of undefined values but can not handle the addition of an undefined value to an existing array, or the addition of a good value to an undefined array. With this change we make the following allowances when reading FITS headers that may have multiple keywords, some of which may be undef: * If a defined value is being added to a key that exists but is undefined, the defined value will override. * If an undef value is being added to a key that exists but is has one or more defined values, the undef value will be dropped. A test has been added to ensure that this is happening.
@TallJimbo as discussed on Slack I've added some code to this PR that means we can read FITS headers that have identical keys but some of which are undefined. I've added a test. I've included warnings and the code looks a bit repetitive so you might want to take a look. |
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.
Code looks fine - yes, it's mildly repetitive, but in a way that makes it quite easy to read, and there's no actual duplication of any significant logic.
If legal FITS files can have both undef and defined values for a particular key (I don't know whether they can), we may not warn to warn about this.
FITS can have cards with identical keys and differing values and sometimes they can be undefined in one place and defined in another. I added the warnings not because FITS can't support that, but because people may be surprised that we can't. |
No description provided.