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

Support for Extensions, Complete Headers and Multiple HDUs #2

Merged
merged 17 commits into from
Aug 28, 2023

Conversation

seanhess
Copy link
Contributor

As described in discussion in #1, adds more robust support for many things in order to parse FITS files from the NSO L1 Data Products.

Since we last spoke I added several things, including full support for BINTABLE, a method to read specific headers, and multiple refactors to better support extensions. Sorry for such a huge PR. Turns out our FITS files used just about every features in the spec!

There are still a few cleanup things to do, comments, formatting, etc, but I thought it would be good to get the PR in.

@seanhess
Copy link
Contributor Author

I'm the most unsure about the Data.Fits.Read module, which was a last minute addition. It makes it easy to parse the file and read specific headers in the Either String monad.

The Either String approach has the advantage of being intuitive and easily combinable with other validators. One alternative is to expose our own Parser monad (a-la-Aeson), and/or support parsing into Generic records. With a custom monad we could carry the HDU as context instead of passing it to the parsing functions, making it ever so slighly cleaner. The main disadvantage of these approaches is end-user complexity, possible confusion with the Megaparsec Parser, and preventing them from using Either String to perform other validations at the same time.

I think maybe the correct design won't emerge until we start writing real code against the library, which I will be doing in within the next month.

@seanhess
Copy link
Contributor Author

@krakrjak Have you had a chance to look at this?

src/Data/Fits.hs Outdated Show resolved Hide resolved
src/Data/Fits.hs Outdated Show resolved Hide resolved
@seanhess
Copy link
Contributor Author

Pushed a change of all Natural -> Int.

Any thoughts on the larger refactors?

@krakrjak krakrjak self-assigned this Aug 28, 2023
@krakrjak krakrjak added the enhancement New feature or request label Aug 28, 2023
@krakrjak krakrjak linked an issue Aug 28, 2023 that may be closed by this pull request
Copy link
Owner

@krakrjak krakrjak left a comment

Choose a reason for hiding this comment

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

I'd like to see a follow up PR where every top-level binding has a comment, but after reviewing this code and playing around a bit, I'm happy to accept it as is.

Thank you @seanhess for your contribution and may this just be the start of something great to allow this library to scratch more itches than when I started down the path many years ago.

Great work, let's do more to make this very useful for others down the road. I look forward to further collaboration.

@krakrjak krakrjak merged commit dbdf77c into krakrjak:master Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for extensions? Good place to start?
2 participants