Generalized lightcurves

[matteobachetti]

An attempt at more general time series objects. Not taking for granted counts or count rates, having multiple array attributes, etc.
It might be, for example, spectroscopic or polarimetric light curves with multiple measurements per time point

Documentation for these changes can be found at StingraySoftware/notebooks#73

(Depends on #774) Merged

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (424fa59) 96.18% compared to head (29cf3a0) 94.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   96.18%   94.22%   -1.96%     
==========================================
  Files          43       43              
  Lines        8020     8473     +453     
==========================================
+ Hits         7714     7984     +270     
- Misses        306      489     +183     

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

[pep8speaks]

Hello @matteobachetti! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file stingray/base.py:

Line 92:101: E501 line too long (101 > 100 characters)
Line 908:101: E501 line too long (166 > 100 characters)
Line 947:101: E501 line too long (166 > 100 characters)
Line 964:101: E501 line too long (166 > 100 characters)

• In the file stingray/lightcurve.py:

Line 1490:101: E501 line too long (103 > 100 characters)
Line 1504:101: E501 line too long (103 > 100 characters)
Line 1509:101: E501 line too long (116 > 100 characters)
Line 1519:101: E501 line too long (103 > 100 characters)

Comment last updated at 2023-12-15 13:24:42 UTC

[matteobachetti]

Fixing things for making them work with the Generalized Light curves produced a monster PR. And there are more modifications to do. I will spin off the fixes to smaller PRs, and try to keep this PR on topic.

[mgullik]

Hi @matteobachetti,

this is an important change in the structure of the code. I will probably divide the review into a few steps

_join_timeseries and join method:
Can this method join StingrayTimeseries with EventList objects?
If so, it might be useful to specify it (maybe I missed it)
Does this method take into account when the MJDREF is present only in one or a portion of the objects that are going to be joint?

In lightcurve.py
Is Lightcurve class a child class of StingrayTimeseries?
I don't see any change in the definition of the class Lightcurve, but I see a few methods that are deleted from here, I think this happened because Lightcurve is going to inherit all the methods of StingrayTimeseries, correct?

in crossspectrum.py and powerspectrum.py
a few comments directly in the text about names

I'm going to review the tests later

[matteobachetti]

@mgullik thanks for the review! I fixed all the docstrings. Indeed, many of those methods were moved from EventList and Lightcurve, and the docstrings were left with the original class names.

In general, all child classes (EventList, Lightcurve) inherit the methods from StingrayTimeseries or re-write them if incompatible. join is partially re-written, but e.g. EventList.join does call _join_timeseries. However, note that there is a check that you cannot join different types. If you start from an event list, you cannot merge it with a light curve, even if both of them are children of StingrayTimeseries.

Changes of MJDREF are accounted for. Note that you cannot be a StingrayTimeseries without defining MJDREF, at least as 0 (It is a field automatically defined in the StingrayTimeseries class, see line 1085).

[mgullik]

Hi @matteobachetti

this is the second part of the review.
You find every comment on the code.

I'm still not sure why StingrayTimeseries won't replace the EventList.
If the time array in StingrayTimeseries doesn't have to be well-sampled, it is possible to lead an event file in StingrayTimeseries and then use rebin (dt = float) to produce/transform the StingrayTimeseries into a light curve.
Of course, I don't want to suggest getting rid of EventList and Lightcurve, but I'd like to know if there is any functionality that EventList has and StingrayTimeseries hasn't

[mgullik]

Hi @matteobachetti,

thank you for addressing all my comments, I approve your Pull Request

[dhuppenkothen]

Great work Matteo! I think this is a huge improvement! I have lots of minor nitpicks, a question about the relationship between EventList and TimeSeries, and a question for the future: with this added functionality, I'm expecting that people will want to somehow connect things like RMFs and ARFs to stingray. I know Matteo Lucchini is working on some code to do that. The question is: have you thought about how these would fit into the context of the current objects? Would they be separate objects? Would they get attached to a TimeSeries object as an attribute?

[matteobachetti]

@dhuppenkothen I should have addressed all your comments in one way or the other.
About responses: they could definitely be added as attributes, and then used for whatever the user wants. Or we could just create another Spectrum object, maybe compatible with the equivalent xspec or sherpa objects. I think feedback from people like @matteolucchini1 , actually doing these things, should drive the choice though.

[matteolucchini1]

@matteobachetti You can find the response stuff I've done so far here https://github.com/matteolucchini1/neXTsPec_prototype
Currently what it does is simply take an impulse response in units of count rate as a function of energy and time (although the time axis could easily be e.g. pulse phase or Fourier frequency), and convolve that with response files to give units of count rate as a function instrument channel and time/whatever else. In practice, what I actually chose to do is simply a matrix multiplication pretty much like x-psi, because it's much more straightforward than the xspec/isis/sherpa way of handling things and the computational cost is negligible.

I'm not too familiar with Stingray, but in principle I think it should be possible to adapt my code to have a more robust energy:channel mapping, and then replace the rough_calibration function.

[dhuppenkothen]

I think this is good to go now! Two requests for separate PRs:
(1) add an rst page to the documentation that explains the overall hierarchy of stingray objects, including where TimeSeries fits in and how it relates to LightCurve etc
(2) I haven't looked at the notebooks yet, but it would be useful to make some of the additional functionality explicit there that might be confusing to the user. I'm thinking of the functionality to create binned TimeSeries from EventLists and the different syntax for creating PSDs out of light curves versus time series.

[matteobachetti]

@matteolucchini1 thanks! We have a function that does something similar in HENDRICS: https://github.com/StingraySoftware/HENDRICS/blob/c0e2c68c965cd54404d7f1b859f1ad3bc96aa9f8/hendrics/calibrate.py#L133
It's probable that yours will be more advanced, but feel free to take a look and recycle/propose changes at will