-
Notifications
You must be signed in to change notification settings - Fork 145
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
Generalized lightcurves #754
Conversation
c1f6447
to
f2209a9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Hello @matteobachetti! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-12-15 13:24:42 UTC |
f9f1421
to
e29dcbb
Compare
23694cb
to
ea4bdb1
Compare
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. |
25eaa51
to
faa4af7
Compare
237b7af
to
59ec903
Compare
6b9407c
to
2ddb7a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@mgullik thanks for the review! I fixed all the docstrings. Indeed, many of those methods were moved from In general, all child classes ( 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @matteobachetti,
thank you for addressing all my comments, I approve your Pull Request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@dhuppenkothen I should have addressed all your comments in one way or the other. |
@matteobachetti You can find the response stuff I've done so far here https://github.com/matteolucchini1/neXTsPec_prototype 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@matteolucchini1 thanks! We have a function that does something similar in HENDRICS: https://github.com/StingraySoftware/HENDRICS/blob/c0e2c68c965cd54404d7f1b859f1ad3bc96aa9f8/hendrics/calibrate.py#L133 |
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