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

Fix for Astropy Header not behaving dict-like. #53

Merged
merged 5 commits into from May 18, 2021

Conversation

DinoBektesevic
Copy link
Contributor

Astropy Header overrides the in operator functionality, effect of which is that an ValueError error is raised when comparison value is not an str, an int or a tuple like (str, int).

This breaks Translator.is_keyword_defined functionality for DECam.
Added tests explicitly testing behavior of ObservationInfo when instantiated from Header objects for all translators.

@DinoBektesevic
Copy link
Contributor Author

I gave it a shot: astropy/astropy#11729; but in the end I think they are right. The primary difference is that a ValueError will be raised instead of a KeyError, but otherwise the behavior of in on Header is consistent with other Python built-in types.

I've left the check in translator.py as keyword is None because it seems to me that MetadataTranslator.is_keyword_defined is primarily used within astro_metadata_translator and it looks nicer. The fixes for MetadataAssertHelper were not as nice, but that was the shortest way to implement the double-translation checking as was discussed; without duplicating docstrings or code in the file, or any of the test_[instrument].py files.

I think that this test there is an overkill though. I think that a "better" fix for this might be wrap the is_keyword_defined in a try-except block and add a test to FitsTranslator that approximately looks like:

def test_keyword_defined(self):
    header = Header(self.header)
    self.assertFalse(FitsTranslator.is_keyword_defined(header, None))

but I don't know if you would like that, and like this it's double-checked that when Astropy fixes Headers nothing gets lost in translation either.

@DinoBektesevic DinoBektesevic requested a review from timj May 12, 2021 19:18
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks okay. I'd like the main logic for porting dict to Header to be tweaked. If we keep the method name the same the diff will be much smaller. Please make sure Jenkins passes.

python/astro_metadata_translator/tests.py Outdated Show resolved Hide resolved
python/astro_metadata_translator/tests.py Outdated Show resolved Hide resolved
# Header can not parse multi-line header keys as lists
for key, val in header.items():
if isinstance(val, list):
header[key] = "".join(val)
Copy link
Member

Choose a reason for hiding this comment

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

This can't be right. By definition FITS headers can have multiple entries for the same key so this should convert as multiple repeat keys. If you want to be pedantic about it you should handle blank lines and comments/history explicitly with add_comment etc methods.

I think it would be more accurate to do something like:

header = Header()
for key, val in header.items():
    if key in ("COMMENT", "HISTORY", ""):
        # comment cards are not relevant for header translation
        continue
    values = val if isinstance(val, list) else [val]
    for v in values:
        header.append((key, val))

I haven't tested this but I hope it gives you the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this was very helpful. What we can't do is

Header({"SIMPLE": True, "EXTEND": True, "COMMENT": ["A ", "list"]})

so as long as we unravel the list into individual elements Astropy will handle insertions for us (including special treatment for special keywords like COMMENT, HIERARCH, blank etc.) I just got too stuck into typing the incoming object that I didn't simplify.

python/astro_metadata_translator/tests.py Outdated Show resolved Hide resolved
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Thanks for reorganizing. Looks great.

python/astro_metadata_translator/tests.py Show resolved Hide resolved
@DinoBektesevic DinoBektesevic merged commit c2dde51 into master May 18, 2021
@DinoBektesevic DinoBektesevic deleted the tickets/DM-30093 branch May 18, 2021 19:31
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