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

Fits event lists #834

Merged
merged 19 commits into from
Oct 22, 2024
Merged

Conversation

matteobachetti
Copy link
Member

Introduces a new FITS reader class, that lazy loads the data until a slice is required (in which case, the wanted StingrayTimeseries object is created).

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 92.62821% with 23 lines in your changes missing coverage. Please review.

Project coverage is 79.52%. Comparing base (aee67bb) to head (0f4a16b).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
stingray/io.py 91.24% 19 Missing ⚠️
stingray/events.py 55.55% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (aee67bb) and HEAD (0f4a16b). Click for more details.

HEAD has 13 uploads less than BASE
Flag BASE (aee67bb) HEAD (0f4a16b)
16 3
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #834       +/-   ##
===========================================
- Coverage   96.11%   79.52%   -16.59%     
===========================================
  Files          48       48               
  Lines        9371     9667      +296     
===========================================
- Hits         9007     7688     -1319     
- Misses        364     1979     +1615     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matteobachetti matteobachetti force-pushed the fits_event_lists branch 8 times, most recently from 680db3f to 0b197e8 Compare September 2, 2024 13:42
@matteobachetti matteobachetti marked this pull request as ready for review September 2, 2024 14:51
@matteobachetti matteobachetti force-pushed the fits_event_lists branch 2 times, most recently from f8fe13b to 888aee1 Compare September 30, 2024 19:03
Copy link
Collaborator

@matteolucchini1 matteolucchini1 left a comment

Choose a reason for hiding this comment

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

Other than the obvious requiring docstrings, I think this kind of class in general desperately needs better documentation/notebooks because without it, it is hard for me to look more carefully into what the code is doing/supposed to do.

It would also help users in understanding how to handle event files more easily, I worry that most users (my past self included) will just look at the notebooks and think "I can throw in an event file when I need my timing data and it'll be fine".

@matteobachetti
Copy link
Member Author

@matteolucchini1 thanks for the review! I will work on the API docs, I do realize now that I could have done much better 😅 . I'm just not sure about notebooks or user-facing documentation: this is a function that is more for internal use in the library, something to speed up the loading of small parts of big files.
This will be used by user facing classes (something like LazyLoadedEventLists, gotta find a better name but that's what they will be), but probably not directly from users.

@matteolucchini1
Copy link
Collaborator

@matteobachetti Ah fair enough. I do still think it would be nice to show different ways of loading event files (e.g. here's what happens when you load an heasoft-compatible file, or a generic fits file like in this PR, or whatever other source is supported), but maybe a bunch of that is beyond the scope of the PR. I'll try to dig into the code between today and early next week!

@matteobachetti
Copy link
Member Author

@matteolucchini1 I wrote a small motivation tutorial here: StingraySoftware/notebooks#107

@matteolucchini1
Copy link
Collaborator

I really like that tutorial! I left a couple more minor comments otherwise it's getting close :)

Copy link
Collaborator

@matteolucchini1 matteolucchini1 left a comment

Choose a reason for hiding this comment

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

No more comments! Just conflicts that need resolving now

@matteobachetti matteobachetti added this pull request to the merge queue Oct 22, 2024
Merged via the queue into StingraySoftware:main with commit 28f2479 Oct 22, 2024
14 of 15 checks passed
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