EVENTLISTS [readevent] #47

kashish2210 · 2025-05-17T19:10:03Z

This PR includes the update to events.JL function so that it contains now abstract methods with a constructor, and mainly comprises the light curve function with metadata and filter as per the discussion
majorly work on this #29

lightcurve function included create_lightcurve,EventProperty, AbstractLightCurve ,rebin, calculate_errors, LightCurve, LightCurveMetadata

tagging @matteobachetti @fjebaker @stefanocovino

codecov-commenter · 2025-05-17T19:14:03Z

Codecov Report

Attention: Patch coverage is 68.22430% with 34 lines in your changes missing coverage. Please review.

Project coverage is 90.92%. Comparing base (cddac34) to head (20b4101).
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
src/events.jl	68.22%	34 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   95.24%   90.92%   -4.33%     
==========================================
  Files           3        4       +1     
  Lines         505      617     +112     
==========================================
+ Hits          481      561      +80     
- Misses         24       56      +32

☔ 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.

matteobachetti · 2025-05-17T19:48:30Z

src/events.jl

                end
+
+                # If we found both time and energy data, we can return
+                if !isempty(times) && !isempty(energies)


Note that most event lists do not have an "energy" column, so this will keep going through the different extension needlessly.
When you find the time column, you will also have found all the information you can find about single events (energy, PI, etc.).

matteobachetti · 2025-05-17T19:53:15Z

src/lightcurve.jl

+    if method === :poisson
+        return convert.(T, sqrt.(counts))
+    elseif method === :gaussian
+        return convert.(T, sqrt.(counts .+ 1))


Gaussian errors need to be given by the user, estimating them like this would be wrong and, in any case, it seems to assume they are Poisson (the sqrt only makes sense in that case, and summing 1 is a quick way to give an error bar when the count rate is 0).

fjebaker

This looks generally really good! I think there are just some redundancies and simplifications that ought to be made to help us maintain this code in the future.

We should also talk about tests and how tests should be organised. We can chat about that in the meeting tomorrow. I'd recommend not being too specific with the tests at this stage, else you'll spend most of your time maintaining the tests when you need to make a small change to how something works.

fjebaker · 2025-05-29T21:04:59Z

src/events.jl

+function Base.getindex(ev::EventList, i)
+    if isnothing(ev.energies)
+        return (ev.times[i], nothing)
+    else
+        return (ev.times[i], ev.energies[i])
+    end
+end


In my opinion, this method is redundant. If someone wants to access the times or energies they can do that via times or energies and deal with the underlying vector.

fjebaker · 2025-05-29T21:06:16Z

src/events.jl

+    if !issorted(evt_times)
+        throw(ArgumentError("Event times must be sorted in ascending order"))
+    end
+    if length(evt_times) == 0
+        throw(ArgumentError("Event list is empty"))
+    end


These sorts of checks should happen in the event list constructor. If they are not met (i.e. event list is not valid), the event list should not be allowed to be created.

fjebaker · 2025-05-29T21:07:34Z

src/events.jl

+function get_column(events::EventList, column_name::String)
+    if column_name == "TIME"
+        return events.times
+    elseif column_name == "ENERGY" && !isnothing(events.energies)
+        return events.energies
+    elseif haskey(events.extra_columns, column_name)
+        return events.extra_columns[column_name]
+    else
+        return nothing
+    end
+end


I understand your intention behind this, but I would get rid of this method too. An event list is a Julia object, not a FITS table, so we don't need to support these kinds of accessors. It's more code that would have to be maintained.

fjebaker · 2025-05-29T21:11:19Z

src/lightcurve.jl

+    gaussian_errors::Union{Nothing,Vector{T}}=nothing,
+    tstart::Union{Nothing,Real}=nothing,
+    tstop::Union{Nothing,Real}=nothing,
+    filters::Dict{Symbol,Any}=Dict{Symbol,Any}()


I''d remove this. It isn't clear what it's supposed to be doing, and if it's applying some kind of filter function, then you can just accept the filter function. A dict with the value type Any won't be performant anyway.

fjebaker · 2025-05-29T21:14:07Z

src/lightcurve.jl

+- `tstart`, `tstop`: Time range limits
+- `filters`: Additional filtering criteria
+"""
+function create_lightcurve(


I think this function does too much and should be broken up into a number of smaller functions that this uses.

For example, a method for

Filtering

Binning

Additional properies (e.g. energy means)

Metadata extraction

Be sure to put all the validating parts of the code (e.g., are the errors valid) at the start of the function. There's no point computing the counts if it's going to error there later.

fjebaker

Thanks @kashish2210 this looks fine! Just some small things remaining. The code is getting quite complex, so I'd urge you to split things up into small, reusable functions in future work, but we can clean this up later in different PRs.

fjebaker · 2025-06-02T12:49:59Z

src/events.jl

+        ms = get_mission_support(mission, instrument, epoch)
+        # Use mission-specific energy alternatives if available
+        energy_alternatives = ms.energy_alternatives
+        ms
+    else
+        nothing
+    end


I can't see get_mission_support implemented anywhere? Is this related to your new PR? If so, it should be in that PR, not here.

fjebaker · 2025-06-02T13:00:48Z

src/events.jl

                end
            end
+
+        catch e


Putting a lot of code inside of a try / catch and then having an open ended catch will lead to catching errors you didn't mean to. Only wrap the line you are expecting to potentially throw the error with the try / catch block.

In general, this function is very long with a lot of nested blocks. Maybe consider breaking the logic up into smaller, reusable functions?

fjebaker · 2025-06-02T13:01:54Z

src/lightcurve.jl

+    timebins::Vector{T}
+    bin_edges::Vector{T}
+    counts::Vector{Int}
+    count_error::Vector{T}
+    exposure::Vector{T}
+    properties::Vector{EventProperty}
+    metadata::LightCurveMetadata
+    err_method::Symbol


Can you add documentation for what these fields are? Why is exposure for example a vector and not just a single value, as some might expect?

kashish2210 · 2025-06-02T19:35:09Z

@fjebaker I have made the changes Here is the summary

Specific catch blocks: Replaced broad catch e with specific error handling:

catch header_error for header reading
catch energy_error for energy column reading
catch sector_error for sector column reading
catch hdu_error for HDU access errors
catch table_error for table reading errors

Reduced nesting: Simplified the logic flow by:

Using findfirst() instead of loops for column searching
Extracting the main HDU reading logic to reduce nesting
Using clearer conditional structures

kashish2210 · 2025-06-02T19:51:59Z

yup, this is getting complex, but it is needed for GTI and to cover all events, so there should be no bad events from next pr we will be back on our track

kashish2210 · 2025-06-07T17:44:49Z

@fjebaker, I have only added events here. Should I create a separate PR for the lightcurve? This PR feels a bit complex now. guess

This reverts commit 34ae9e8.

fjebaker

This is great @kashish2210 and thanks for splitting the events list and lightcurve up so that we can review them in small chunks!

Don't add anything new to this PR. If you can just address the changes I've suggested, then from my end I'm happy for this to be merged!

fjebaker · 2025-06-07T20:52:28Z

src/events.jl

+# Fields
+- `filepath::String`: Path to the FITS file
+- `hdu::Int`: HDU index that the metadata was read from
+- `energy_units::Union{Nothing,String}`: Units of energy (column name: ENERGY, PI, or PHA)
+- `extra_columns::Dict{String,Vector}`: Extra columns that were requested during read
+- `headers::H`: FITS headers from the selected HDU


No need to do this here, but you can use DocStringExtensions.jl, specifically the FIELDS or TYPEDFIELDS parameters to generate the field documentation directly.

fjebaker · 2025-06-07T20:53:45Z

src/events.jl

+# Filtering
+EventList supports composable filtering operations:
+```julia
+# Filter by time (in-place)
+filter_time!(t -> t > 100.0, ev)
+
+# Filter by energy (in-place)  
+filter_energy!(energy_val -> energy_val < 10.0, ev)
+
+# Non-mutating versions
+ev_filtered = filter_time(t -> t > 100.0, ev)
+ev_filtered = filter_energy(energy_val -> energy_val < 10.0, ev)
+
+# Composable filtering
+filter_energy!(x -> x < 10.0, filter_time!(t -> t > 100.0, ev))
+```
+


This should be in the docstring for filter_time! and filter_energy! respectively.

src/events.jl

test/test_events.jl

Co-authored-by: Fergus Baker <[email protected]>

kashish2210 · 2025-06-10T17:18:11Z

Hey @matteobachetti Thank you for the feedback, I am replying your comment about function filter_energy(f, ev::EventList) from #51

reason

filter_energy!(f, ev::EventList) - modifies the EventList in-place
filter_energy(f, ev::EventList) - returns a new filtered EventList (non-mutating)
This follows Julia's standard convention where functions ending with ! are mutating (modify their arguments) and functions without ! are non-mutating (return new objects). The same pattern applies to filter_time! vs filter_time.
However, I understand your suggestion about using a keyword argument instead. We could consolidate this into a single function like:

function filter_energy(f, ev::EventList; inplace::Bool = false)
    if inplace
        return filter_energy!(f, ev)
    else
        new_ev = deepcopy(ev)
        return filter_energy!(f, new_ev)
    end
end

@fjebaker @matteobachetti shall I implement this if you guys say i will add this in this PR

matteobachetti · 2025-06-10T17:25:17Z

filter_energy!(f, ev::EventList) - modifies the EventList in-place filter_energy(f, ev::EventList) - returns a new filtered EventList (non-mutating) This follows Julia's standard convention where functions ending with ! are mutating (modify their arguments) and functions without ! are non-mutating (return new objects). The same pattern applies to filter_time! vs filter_time.

Oh! I see. If that's Julia standard, I'm totally ok with it, I missed the ! character and I didn't understand what was going on. Actually I should have known because it was mentioned in Fergus' classes 😅

kashish2210 · 2025-06-10T17:27:15Z

Same this was one of @fjebaker implementation from #50

matteobachetti

Ok for me, I think it's a good starting point for the rest of the open PRs

fjebaker

Nice work! 👍

kashish2210 · 2025-06-13T12:52:47Z

thanks

kashish2210 added 2 commits May 18, 2025 00:31

add abstract metod to envent reading

58b40b1

add lightcurve function

847039b

add some more test for events.jl as per codcov coverage

8e5001b

matteobachetti reviewed May 17, 2025

View reviewed changes

kashish2210 added 2 commits May 22, 2025 01:29

major update in event.jl

f50f3df

upadtemin error methor for light curve

686bb6b

kashish2210 requested a review from matteobachetti May 22, 2025 19:35

fjebaker reviewed May 29, 2025

View reviewed changes

kashish2210 added 2 commits June 2, 2025 03:12

update

c48a6d0

constructor update for sorting events validation

3c9f7a3

fjebaker reviewed Jun 2, 2025

View reviewed changes

fix try and catch block and add docstring

583f352

kashish2210 requested a review from fjebaker June 2, 2025 19:35

kashish2210 mentioned this pull request Jun 2, 2025

Mission support #49

Draft

fjebaker mentioned this pull request Jun 3, 2025

Example: Simple EventList implementation #50

Draft

adding updated event.jl and test cases refering StingraySoftware#50

1da0960

kashish2210 changed the title ~~Lightcurve and abstractEvents~~ EVENTLISTS [readevent] Jun 7, 2025

kashish2210 marked this pull request as draft June 7, 2025 17:49

kashish2210 marked this pull request as ready for review June 7, 2025 17:50

kashish2210 added 2 commits June 8, 2025 01:47

add lightcurve

34ae9e8

Revert "add lightcurve"

510febc

This reverts commit 34ae9e8.

kashish2210 mentioned this pull request Jun 7, 2025

LIGHTCURVE(CREATE_LIGHTCURVE, REBIN) #51

Merged

fjebaker requested changes Jun 7, 2025

View reviewed changes

kashish2210 and others added 2 commits June 8, 2025 02:37

Update test/test_events.jl

7c57639

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

ae297ba

Co-authored-by: Fergus Baker <[email protected]>

kashish2210 and others added 17 commits June 8, 2025 02:37

Update test/test_events.jl

68585b8

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

24bfa63

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

2f17a7c

Co-authored-by: Fergus Baker <[email protected]>

Update src/events.jl

0c0b4fd

Co-authored-by: Fergus Baker <[email protected]>

Update src/events.jl

f2a8653

Co-authored-by: Fergus Baker <[email protected]>

Update src/events.jl

d209748

Co-authored-by: Fergus Baker <[email protected]>

Update src/events.jl

c869a2d

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

f68ea52

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

3d7927d

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

deb8f5b

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

8bcc5ae

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

ae6582d

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

fee9048

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

5f77b5b

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

932cc58

Co-authored-by: Fergus Baker <[email protected]>

Update test/test_events.jl

b0b8ec6

Co-authored-by: Fergus Baker <[email protected]>

update docsting

20b4101

kashish2210 requested a review from fjebaker June 7, 2025 21:34

matteobachetti approved these changes Jun 11, 2025

View reviewed changes

fjebaker approved these changes Jun 13, 2025

View reviewed changes

fjebaker merged commit 95d8630 into StingraySoftware:main Jun 13, 2025
2 checks passed

kashish2210 deleted the lightcurve branch June 13, 2025 12:52

kashish2210 mentioned this pull request Jun 15, 2025

fix_issue#34 #38

Closed

kashish2210 mentioned this pull request Jul 3, 2025

Events are typically only in one FITS extension #34

Closed

                               end
                           end
+                      catch e

EVENTLISTS [readevent] #47

EVENTLISTS [readevent] #47

Uh oh!

Conversation

kashish2210 commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjebaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjebaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashish2210 commented Jun 2, 2025

Specific catch blocks: Replaced broad catch e with specific error handling:

Reduced nesting: Simplified the logic flow by:

Uh oh!

kashish2210 commented Jun 2, 2025

Uh oh!

kashish2210 commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjebaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kashish2210 commented Jun 10, 2025

reason

Uh oh!

matteobachetti commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kashish2210 commented Jun 10, 2025

Uh oh!

matteobachetti left a comment

Choose a reason for hiding this comment

Uh oh!

fjebaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kashish2210 commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

kashish2210 commented May 17, 2025 •

edited

Loading

codecov-commenter commented May 17, 2025 •

edited

Loading

kashish2210 commented Jun 7, 2025 •

edited

Loading

matteobachetti commented Jun 10, 2025 •

edited

Loading