seanhess
commented
1 year ago Hey @krakrjak are you around? Would you be willing to accept a PR for fits-parse with support for header extensions?
Closed seanhess closed 1 year ago
seanhess
commented
1 year ago Hey @krakrjak are you around? Would you be willing to accept a PR for fits-parse with support for header extensions?
krakrjak
commented
1 year ago Hi @seanhess I am here and I'm very enthusiastic about you reaching out. I have thought a lot about these header extensions, but I stopped with this package basically at MVP where it supports the standard, but not any extensions (yet). I certainly welcome ideas, PRs, designs, and any concepts you might have for supporting these things. I have no objection to supporting a much larger variety of FITS formats and extensions.
I'm all ears! Also, I haven't updated this guy in years! I happy to see someone is poking at it. I guess I should dust it off and make a fresh release.
So tell me @seanhess got any opening ideas of where to start?
seanhess
commented
1 year ago Glad to hear from you! I’ll focus on extension header parsing first, since our fits files don’t load. I should have something early next week.
My fork has a small sample of one of our files if you want to poke around or beat me to anything.
Our image frames have 2 HDUs. The first doesn’t have much in it besides basic metadata, and no data. The second has a bunch of headers and the pixel data.
https://github.com/seanhess/fits-parse/tree/master/fits_files
Looking forward to collaborating! I’ll let you know as soon as I have thought about design changes, and I’d love to hear your thoughts on supporting custom headers if you want.
seanhess
commented
1 year ago Ok @krakrjak first refactoring question. What if we try to get parsed types into a more haskell-friendly format? For example, we currently have StringValue = { type = Null | Empty | Data, value = Maybe Text }
Would you be opposed to making it simply Maybe Text? It can represent all of those states: (Nothing, Just "", Just data).
I haven't gone through NumberType carefully, but it might be better represented with a single ADT:
data NumberValue
= NumInt Int
| NumReal Double
| NumImaginary SomeImaginaryNumberTypeAny thoughts?
seanhess
commented
1 year ago The basic theory behind the above is to "Make invalid states unrepresentable". StringValue as defined can be in an inconsistent state: { type = Data, value = Nothing }, or { type = Null, value = Just "asdf" }. This means that any downstream code has to handle possible failure cases at every use site.
seanhess
commented
1 year ago @krakrjak I've made some major changes in my fork to support extensions, will you take a look at let me know what you think about the direction?
What would you like to change?
EDIT: I'm not 100% sure the header parsing will be ergonomic. The NSO file still isn't parsing because we need support for COMMENT keywords first. Afterwards, I'll try to write some code against it and see how it feels. It would be nice to have a single parsing pass into custom Haskell types, but we'll see.
krakrjak
commented
1 year ago My sincere apologies that I have not given any feedback yet on your changes. I am traveling for work this week and I have not found the time to sit down with this code.
At first blush, I think you are headed in the right direction. Supporting multiple HDUs is the main trick. Beyond that we might have to be more specific in the parser rather than more general, but I don't really have an intuition here yet. I may not get a solid chance to look at the code until the weekend.
seanhess
commented
1 year ago Hey @krakrjak, good timing! I just got my changes to a point where I felt comfortable creating a PR. I'll wait until you've had a chance to review before doing anything else.
I'm looking forward to hearing your comments and ideas.
krakrjak
commented
1 year ago Looks really good. Thank you so much for your contribution and waiting on me. I know it's not easy, I've had a strange two weeks with travel and COVID. I'm about to finish the review. Likely no more changes are needed, but I'm double checking a few things to make sure.
seanhess
commented
1 year ago Hooray! Need help making a new release on Hackage?
On Sun, Aug 27, 2023 at 6:48 PM Zac Slade @.***> wrote:
Closed #1 https://github.com/krakrjak/fits-parse/issues/1 as completed via #2 https://github.com/krakrjak/fits-parse/pull/2.
— Reply to this email directly, view it on GitHub https://github.com/krakrjak/fits-parse/issues/1#event-10203460726, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAD66P3JHLHELMTPQZUTKLXXP2HTANCNFSM6AAAAAA3KIXQHY . You are receiving this because you were mentioned.Message ID: @.***>
Hi there! I just started at the National Solar Observatory, and I'm hoping to use Haskell to produce our Level 2 data.
I'm going to need to leverage header extensions. Have you thought about them at all yet? Would you mind letting me know any plans / thoughts / warnings I might need if I were to try to create a PR supporting them?
Below is an example header I'll need to parse.
Thanks for your work on this package!