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

detect gzipped-FITS files by magic-bytes #6693

Merged
merged 10 commits into from
Jan 7, 2023
Merged

Conversation

exitflynn
Copy link
Member

PR Description

Detection for gzipped-FITS files is now done by checking file signatures instead of checking for file extensions.
First a gzip file-signature is checked and if found, the file's first few bytes are decompressed and checked for the FITS file signature.
The PR closes #6692.

@exitflynn exitflynn requested a review from a team as a code owner January 5, 2023 21:26
@nabobalis nabobalis added io Affects the io submodule BugFix labels Jan 5, 2023
@nabobalis
Copy link
Contributor

Hi @exitflynn, thanks for the PR. If you could add a unit test for this in https://github.com/sunpy/sunpy/blob/main/sunpy/io/tests/test_filetools.py

There should be some gzipped files to use in https://github.com/sunpy/sunpy/tree/main/sunpy/data/test

Co-authored-by: Conor MacBride <[email protected]>
@exitflynn
Copy link
Member Author

sure. i'll be getting to including PKZIP and BZIP2 detection and the tests tomorrow! (it's quite late here)

@nabobalis
Copy link
Contributor

No problem. I also wouldn't add the other two file formats for now.

@exitflynn
Copy link
Member Author

Hi, I just noticed that a test for detecting gzipped FITS files already exists, since i only re-modified how this was implemented.

@exitflynn
Copy link
Member Author

also i'm not sure why the CI tests have failed, is it something unrelated to my changes that I can ignore? I tried runing the tests before pushing by running test_filetools.py and it didn't raise any issue (this is my first time testing stuff).

@Cadair
Copy link
Member

Cadair commented Jan 6, 2023

Hi, I just noticed that a test for detecting gzipped FITS files already exists, since i only re-modified how this was implemented.

Could you add a test where the filextension doesn't match but we still detect it properly? This means we test this new behaviour and we make sure that we don't regress in the future?

sunpy/io/file_tools.py Outdated Show resolved Hide resolved
@Cadair
Copy link
Member

Cadair commented Jan 6, 2023

also i'm not sure why the CI tests have failed, is it something unrelated to my changes that I can ignore? I tried runing the tests before pushing by running test_filetools.py and it didn't raise any issue (this is my first time testing stuff).

It's not your fault, if you can rebase or merge in main then it should be fixed now #6694

@exitflynn
Copy link
Member Author

right now there are two tests for gzipped-fits files, the test_read_file_fits_gzip() which tests filename.fits.gz, -.fts.gz and -fit.gz files (namely the ones that were included there when detection was done based on the extension) and the test_read_file_fits_gzip_not_gz_extension() which tests gzip_fits_test.file.

I think it'd make more sense to have only one test for gzip-fits files, the latter one since if it passes then there's no need to test for the first one.

Should I remove the first test (renaming the second one to test_read_file_fits_gzip())? If yes, it'd also make the three test -fits.gz files redundant, should i delete those too?

@nabobalis
Copy link
Contributor

If you can combine the two tests, I would prefer that.

@exitflynn
Copy link
Member Author

in that case, is combining them like that alright?

@nabobalis
Copy link
Contributor

Looks fine to me.

@nabobalis
Copy link
Contributor

If you could add a bugfix changelog following https://github.com/sunpy/sunpy/blob/main/changelog/README.rst

and if you could also deal with the pre-commit.ci fail.
You can do that either installing pre-commit, running the tox -e codestyle or checking the failure and fixing it manually.

@exitflynn
Copy link
Member Author

yep, done! (I mean I think that should do it? I can't figure out what's causing the codecov/project check to fail... 🤔 )

changelog/6693.feature.rest Outdated Show resolved Hide resolved
changelog/6693.feature.rest Outdated Show resolved Hide resolved
exitflynn and others added 2 commits January 7, 2023 21:01
@exitflynn
Copy link
Member Author

thanks @ConorMacBride! done 👍

Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

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

Thanks @exitflynn!

@exitflynn
Copy link
Member Author

🤠 Thanks Conor, Nabil and Stuart for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Affects the io submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve gzipped-FITS files detection
4 participants