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

Only keep Header values from the Primary that are not present in extension. #6489

Merged
merged 1 commit into from Sep 14, 2017
Merged

Only keep Header values from the Primary that are not present in extension. #6489

merged 1 commit into from Sep 14, 2017

Conversation

MSeifert04
Copy link
Contributor

In case a CCDData is read from a file and the primary HDU does not contain an image it will search for the first extension with data. In that case the Header is combined, but previously useful values like NAXIS, etc. were discarded in that operation.

@astropy-bot
Copy link

astropy-bot bot commented Aug 29, 2017

Hi there @MSeifert04 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

@pllim
Copy link
Member

pllim commented Aug 30, 2017

Do you check for INHERIT=T before actually inheriting primary header keywords? https://fits.gsfc.nasa.gov/registry/inherit.html

@MSeifert04
Copy link
Contributor Author

MSeifert04 commented Aug 30, 2017

No 😓

That PR just makes sure the primary header values have no precedence over the extension header values (now it's the other way around). I think this is now an "always INHERIT=T".

@pllim
Copy link
Member

pllim commented Aug 30, 2017

I haven't see any actual file with INHERIT=F either but it is always a possibility. I guess that is a separate issue from this PR though.

@MSeifert04
Copy link
Contributor Author

I created an issue for it. It also affects other paths of the ccddata_fits_reader function and could break backwards-compatibility so I won't deal with it here.

This PR is somewhat needed to fix the ccdproc test suite (which fails against master) so I would like this to be part of 2.0.2 :)

@MSeifert04
Copy link
Contributor Author

MSeifert04 commented Sep 7, 2017

@mwcraig do you have time to review this?

@bsipocz bsipocz modified the milestones: v2.0.2, v2.0.3 Sep 8, 2017
@MSeifert04
Copy link
Contributor Author

MSeifert04 commented Sep 8, 2017

@bsipocz This really should be in 2.0.2 (if someone reviews it) because ccdproc kind of relies on it.

@bsipocz
Copy link
Member

bsipocz commented Sep 8, 2017

@MSeifert04 - I'm afraid this also missed the release, but we probably can have a 2.0.3 soon after the CC meeting.

@MSeifert04
Copy link
Contributor Author

okay, let me know as soon as there is a 2.0.3 section in the changelog. I'll move the changelog then. :)

@bsipocz
Copy link
Member

bsipocz commented Sep 8, 2017

I'm waiting for the wheel builds to kick off, so hopefully within an our can push up the new changelog, etc.

@bsipocz
Copy link
Member

bsipocz commented Sep 8, 2017

@MSeifert04 - the changelog section is now in master.

@MSeifert04
Copy link
Contributor Author

@mwcraig @crawfordsm Could you have a look? We merged a similar PR in ccdproc already.

@pllim
Copy link
Member

pllim commented Sep 12, 2017

There are some conflicts -- FYI

…nsion.

In case a CCDData is read from a file and the primary HDU does
not contain an image it will search for the first extension with
data. In that case the Header is combined, but previously useful
values like NAXIS, etc. were discarded in that operation.
@MSeifert04
Copy link
Contributor Author

MSeifert04 commented Sep 12, 2017

Thanks for the heads up @pllim , conflicts should be fixed now :)

@MSeifert04 MSeifert04 deleted the ccddata_fix_problem_with_header_combine_reader branch September 14, 2017 18:44
bsipocz pushed a commit that referenced this pull request Sep 26, 2017
…der_combine_reader

Only keep Header values from the Primary that are not present in extension.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants