Implement EventList #7
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7 +/- ##
===========================================
- Coverage 95.24% 21.43% -73.81%
===========================================
Files 3 6 +3
Lines 505 667 +162
===========================================
- Hits 481 143 -338
- Misses 24 524 +500
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Aman-Pandey-afk
force-pushed
the
eventlist
branch
from
96db379 to
205e78d
Compare
Aman-Pandey-afk
force-pushed
the
eventlist
branch
from
205e78d to
6daf452
Compare
|
These were minor mistakes (like not uncommenting some codes, mostly 1-2 lines) so force pushed in the same commit. |
| timesys::String="" | ||
| end | ||
|
|
||
| function read(::Type{EventList},filename::String, format::String) |
There was a problem hiding this comment.
I presume the first argument is to indicate the output type, right? Most read methods have it as last argument, not first one
| end | ||
|
|
||
| function from_lc(lc::LightCurve) | ||
| times = [lc.time[i] for i in 1:length(lc.time) for _ in 1:lc.counts[i]] |
There was a problem hiding this comment.
It's preferable to iterate over the actual indices of an array with eachindex
| times = [lc.time[i] for i in 1:length(lc.time) for _ in 1:lc.counts[i]] | |
| times = [lc.time[i] for i in eachindex(lc.time, lc.counts) for _ in 1:lc.counts[i]] |
In this way you also avoid the assumption that arrays are 1-based indexed, which is not necessarily the case.
| tstart=0 | ||
| tseg=0 |
There was a problem hiding this comment.
I think ev.git has typically Float64 values?
Line 10 in cdeaf8e
zero(eltype(ev.gti)).
| tstart = ev.gti[begin][begin] | ||
| tseg = ev.gti[end][end]-tstart |
There was a problem hiding this comment.
Now that I look closer to this, I'm a bit confused by the ev.gti[index][index] notation: what do you want to do? That's not how indexing of multi-dimensional arrays (like matrices) work.
| n = 1384132 | ||
| mean_counts = 2.0 | ||
| times = range(dt/2, dt/2 + n*dt, step = dt) | ||
| counts = zero(times) .+ mean_counts |
There was a problem hiding this comment.
Not performance-critical in the tests, but I think it makes more sense to just create a vector with all mean_counts instead of creating a vector of zeros and adding mean_counts to it?
| counts = zero(times) .+ mean_counts | |
| counts = fill(mean_counts, size(times)) |
This also saves the allocation of one array.
| dt_new = 1.5 | ||
| lc_binned = rebin(lc, dt_new) | ||
| @test lc_binned.dt == dt_new | ||
| counts_test = zero(lc_binned.time) .+ lc.counts[1]*dt_new/lc.dt |
There was a problem hiding this comment.
| counts_test = zero(lc_binned.time) .+ lc.counts[1]*dt_new/lc.dt | |
| counts_test = fill(lc.counts[begin] * dt_new / lc.dt, size(lc_binned.time)) |
| FITS(filename) do hduList | ||
| eventHDU = hduList["EVENTS"] | ||
| cols = FITSIO.colnames(eventHDU) | ||
| df = DataFrame(eventHDU) |
There was a problem hiding this comment.
Why going through a dataframe? That looks unnecessary? For example, you can get the "TIME" column with read(eventHDU, "TIME").
| y_err::AbstractVector{<:Real}=Float64[], method::String = "sum", dx::Real=0) | ||
| if isempty(y_err) | ||
| y_err = zero(y) | ||
| end | ||
|
|
||
| if dx isa AbstractVector |
There was a problem hiding this comment.
How can dx be an AbstractVector if it's annotated as Real?
|
|
||
| if method in ["mean", "avg", "average", "arithmetic mean"] | ||
| ybin = output / step_size | ||
| ybinerr = sqrt.(outputerr) / step_size |
There was a problem hiding this comment.
Fuse dot operations
| ybinerr = sqrt.(outputerr) / step_size | |
| ybinerr = sqrt.(outputerr) /. step_size |
| end | ||
|
|
||
| new_x0 = (x[1] - (0.5 * dx_old[1])) + (0.5 * dx_new) | ||
| xbin = 1:length(ybin) * dx_new .+ new_x0 |
There was a problem hiding this comment.
Just double checking: are you entirely sure this is parsed as you expect?
3 participants
This is a basic implementation of
EventListwith essential APIs and testsThings remaining:
Cross-spectra/Periodograms APIs and documentation will be implemented soon in next PRs