Skip to content

Conversation

@fjebaker
Copy link
Collaborator

@fjebaker fjebaker commented Jun 3, 2025

@kashish2210 I've implemented a small EventList in this PR that can read all of the standard stringray test files.

I'm not suggesting we use this implementation, but I'd encourage you to look at it as an example for how you could simplify some of the logic in your implementation in #47.

Some things to note:

  • By having the metadata field hold everything "complex", we can later add dummy metadata and write tests that don't need to load in data from a mock FITS file, but just use a EventList(times, energies) constructor directly.
  • Separating the read_energy_column function simplifies the logic of reading the energy column dramatically. The same thing is happening, but the code doesn't need to define variables ahead of time, and the read_energy_column function can be tested independently if needed.
  • Supplying a handful of ::Vector{T} or ::FITSIO.FITSHeader annotations means that this code is also completely type stable when reading in data (you can check that with @code_warntype).
  • You mentioned there needed to be complexity for the handling of GTIs. I've implemented a filter_time! function to illustrate how you could write a composable small function that modifies the event list in-place, which can later be used to filter events in a set of GTIs. You could do similar for a filter_time or such function that would return a new event lists with events that match the predicate. The benefit of writing these like so is now someone else could e.g. filter events after some given time by just doing filter_time!(>(min_time), event_list).

With Unitful.jl, this could even be more expressive:

filter_energy!(<(10u"keV"), filter_time!(>(min_time * u"s"), event_list))

Regarding tests:

  • The tests are not wrapped in @testset outside of runtests.jl. This means you can interactively run the test within your IDE of choice and fix things simply.
  • The tests are not as detailed as yours, which has pros and cons, but are checking the broad strokes of how data is being read in. The idea then would be to add new test cases as bugs are encountered.
  • There's also no tests for the filtering, but that's just because this is an example.

More on GTIs:

  • GTIs are typically also in the events FITS file, and are normally just a list of two columns START and STOP, which ought to be read at the same time. That means we should augment EventList with the GTI information somehow, for example, adding a gti_start and gti_stop field of vectors. But that should be a seperate PR.

fjebaker added 7 commits June 3, 2025 15:52
Simplified the logic for reading in data and let the libraries throw
their errors back to the user to handle. Implemented a very basic
`filter!` function as demonstration for how this structure can be used,
and to add a simple API for when GTIs are tied together with the event
list.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 40.00000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 89.37%. Comparing base (cddac34) to head (7f382e6).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
src/events.jl 40.00% 39 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (cddac34) and HEAD (7f382e6). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (cddac34) HEAD (7f382e6)
6 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   95.24%   89.37%   -5.88%     
==========================================
  Files           3        4       +1     
  Lines         505      574      +69     
==========================================
+ Hits          481      513      +32     
- Misses         24       61      +37     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kashish2210
Copy link
Member

I will start working like this for this week @fjebaker thanks for your reference :)

# modified from the Base.filter! implementation
j = firstindex(ev.times)

for i in eachindex(ev.times)
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eachindex gives you a lazy iterator that goes over all of the indices of an array. If your array has unusual indexing (e.g. starts at 0 or something else, or maybe uses IndexCartesian), it gives you something that will hit every element.

In this case, it's equivalent to firstindex(ev.times):lastindex(ev.times), or equivalently 1:length(ev.times), but would work with other array types just fine.

Somewhere in the Julia docs it says to prefer eachindex over 1:length so that code is more portable. In this case it really doesn't make a difference. So many ways to skin a cat!

kashish2210 added a commit to kashish2210/Stingray.jl that referenced this pull request Jun 7, 2025
kashish2210 added a commit to kashish2210/Stingray.jl that referenced this pull request Jun 15, 2025
kashish2210 added a commit to kashish2210/Stingray.jl that referenced this pull request Jul 2, 2025
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.

4 participants