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

DM-26946: readFits in calibType.ptc seems to fail to read more than one table #158

Merged
merged 1 commit into from Oct 9, 2020

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Oct 7, 2020

Handle multi-extension tables correctly.

@czwa czwa requested a review from plazas October 7, 2020 22:57
Copy link
Contributor

@plazas plazas left a comment

Choose a reason for hiding this comment

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

It looks good to me. The only comment I have is, it seems that it stops when it fails because there are no more tables to read. Could it be possible to have a few good tables, then a bad table, and then more good tables that won't get read because it quit at the first bad table that it encountered? Maybe this is something that actually doesn't happen.

@czwa
Copy link
Contributor Author

czwa commented Oct 8, 2020

It looks good to me. The only comment I have is, it seems that it stops when it fails because there are no more tables to read. Could it be possible to have a few good tables, then a bad table, and then more good tables that won't get read because it quit at the first bad table that it encountered? Maybe this is something that actually doesn't happen.

It's possible, but I think unlikely (and may only be possible if there's some file corruption). The main issue is that astropy Table doesn't have access to the full HDUList, so there's no simple way to know how many tables there are.

@czwa czwa merged commit 206d22f into master Oct 9, 2020
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