Skip to content

Add support for opening FITS files with fsspec#16

Open
derekocallaghan wants to merge 3 commits intoUSRA-STI:mainfrom
GIFTS-6U:fits_open_use_fsspec
Open

Add support for opening FITS files with fsspec#16
derekocallaghan wants to merge 3 commits intoUSRA-STI:mainfrom
GIFTS-6U:fits_open_use_fsspec

Conversation

@derekocallaghan
Copy link
Contributor

gdt.core.file.FitsFileContextManager uses astropy.io.fits.open() to open local FITS files. This PR adds support for using fsspec to open FITS files stored remotely, removing the requirement for prior FITS retrieval and local storage, while also automatically caching the files if required. This is achieved with the use_fsspec and fsspec_kwargs parameters supported by astropy.io.fits.open()

A couple of test cases have been added that use fsspec to open and cache GBM catalog FITS files.

@derekocallaghan
Copy link
Contributor Author

Hi @AdamGoldstein-USRA and @BillCleveland-USRA,

Just checking if any more info or changes are required for this PR?

Thanks

@BillCleveland-USRA
Copy link
Contributor

I'll look over the code and may approve with the caveat being that an optional dependency would need to be installed prior to use.

@BillCleveland-USRA
Copy link
Contributor

The changes you committed today has merge conflict. It will need to be corrected at the command line.

@derekocallaghan
Copy link
Contributor Author

I'm unsure what the issue is here, I've manually copied the main version of test_file.py and reapplied the changes, and run test_file successfully with pytest. The conflict link above seems to have a problem with merely adding a couple of functions at the end. I can try adding them to the class-based unit tests in case that's the issue.

@BillCleveland-USRA
Copy link
Contributor

In the future, do not add updates from main to your pull request unless directed. It muddies up the change history.

@BillCleveland-USRA
Copy link
Contributor

I'll take a look at it and fix it.

@derekocallaghan
Copy link
Contributor Author

derekocallaghan commented Mar 19, 2024

I'll take a look at it and fix it.

Thanks, I've moved the test function into the class, it's running locally but still has a conflict.

@derekocallaghan
Copy link
Contributor Author

In the future, do not add updates from main to your pull request unless directed. It muddies up the change history.

No problem. I wouldn't normally do this but originally did it earlier in an attempt to resolve the conflict.

@derekocallaghan
Copy link
Contributor Author

I'll look over the code and may approve with the caveat being that an optional dependency would need to be installed prior to use.

Would you like me to add fsspec to requirements.txt?

@derekocallaghan
Copy link
Contributor Author

derekocallaghan commented Mar 19, 2024

I'll take a look at it and fix it.

Thanks, I've moved the test function into the class, it's running locally but still has a conflict.

I'd like to try one more thing, i.e. the class-based test function without the recent main changes. That should ensure the history is clean, assuming it works.

@BillCleveland-USRA
Copy link
Contributor

Would you like me to add fsspec to requirements.txt?

No.

@BillCleveland-USRA
Copy link
Contributor

I'd like to try one more thing, i.e. the class-based test function without the recent main changes. That should ensure the history is clean, assuming it works.

Ok.

derekocallaghan and others added 2 commits March 19, 2024 18:39
Signed-off-by: Derek O'Callaghan <derekocallaghan@users.noreply.github.com>
@derekocallaghan
Copy link
Contributor Author

I reverted to the original PR commit and manually resolved the test_file.py conflict here. There doesn't appear to be any conflict merging this commit of core.file with the corresponding recent main changes, so the commit history should be clean for the latter.

@derekocallaghan
Copy link
Contributor Author

Apologies, it looks like GitHub has merged the main changes following manual edit of test_file.py here. If you prefer, I can revert to the original PR commit and try again.

@BillCleveland-USRA
Copy link
Contributor

That's ok. I'm going to look at the current pull and offer some suggestions when I'm done with the review.

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.

2 participants