-
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
Astronomy Using Unevenly Sampled Data : GSoC 2023 #737
Astronomy Using Unevenly Sampled Data : GSoC 2023 #737
Conversation
The |
Codecov Report
@@ Coverage Diff @@
## main #737 +/- ##
==========================================
+ Coverage 96.62% 97.34% +0.71%
==========================================
Files 41 42 +1
Lines 7388 7649 +261
==========================================
+ Hits 7139 7446 +307
+ Misses 249 203 -46
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@pupperemeritus a small suggestion: when you incorporate recent additions to main, please rebase instead of merging/pulling. This will avoid all the merge commits polluting the history. |
I will do that 👍 |
58097e6
to
b5998dc
Compare
b3d9ed1
to
59b2aa5
Compare
f013cc8
to
d2d7968
Compare
42b8646
to
ef71402
Compare
b227128
to
a9d66a3
Compare
cdb9428
to
8428ecd
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.
@pupperemeritus thanks for your work!
It's overall good, I left a few comments about things that can be improved.
Phase lags should work! I added a test that shows that, at least when the time arrays are the same, phase lags should be measured correctly.
I will make those changes |
bc23d38
to
8ceaeab
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.
Thanks @pupperemeritus. Please note that the _make_crossspectrum is the legacy interface in Stingray, and that much of the checks on EventLists are currently wrong and can be simplified.
As a final change, I would like to ask you to please use the from_events, from_lightcurves, etc. functions, to uniformize to the most recent versions of AveragedCrossspectrum. When implementing from_events, you will need to create light curves internally using a given sample time dt
.
stingray/lombscargle.py
Outdated
): | ||
raise TypeError("Both the events are not of type Eventlist or Lightcurve or None") | ||
|
||
if type(data1) == type(data2): |
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.
Instead of raising ValueError, I would just say that the lags will not be correct. The process should actually work.
stingray/lombscargle.py
Outdated
if len(data1.time) != len(data2.time): | ||
raise ValueError("data1 and data2 must have the same length") | ||
else: | ||
if (isinstance(data1, EventList) or isinstance(data2, EventList)) and ( |
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.
The logic here is wrong. If you have event lists, the time arrays will always be different.
stingray/lombscargle.py
Outdated
if not isinstance(fullspec, bool): | ||
raise TypeError("fullspec must be a boolean") | ||
|
||
if np.logical_xor( |
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.
In general, I'd say you are overcomplicating the data type selection here. You have various instances of isinstance
filtering that should be simplified. What don't you like of the machinery in the standard Crossspectrum?
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 it was something along the lines of EventList, Lightcurve intermixing while accounting for mixed type with one none. I think that train of thought went quiet overcomplicated. I guess I will simplify and bring it in line with the regular crossspectrum
dad252a
to
bf96713
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.
@pupperemeritus great new additions! Some parts of the code are not tested yet. I added an example test that should pass if the EventList machinery works, there should be a few more, some making the code run correctly and testing its values, some making it break in predictable ways. Please try to get the patch coverage to 100%
…ragedCrossspectrum
Co-authored-by: Matteo Bachetti <[email protected]>
43474dc
to
c55c0d6
Compare
@astrofrog @jakevdp would the Lomb-Scargle cross spectrum be interesting to be put in astropy timeseries? @pupperemeritus's work might be useful for a more general audience than ours. |
@pupperemeritus, thanks for your hard work! This makes an excellent addition to Stingray, and it will be one of the highlights of the next release. |
Thank you @matteobachetti. It was a fun working on it and a great learning experience. |
This is a work in progress. Feel free to provide critique and suggestions for any bugs that may be or incorrect behaviour. So far I have implemented the base Lomb-Scargle Cross Spectrum and Power Spectrum classes. Do mention any useful statistical functions that may be of interest to people dealing with unevenly sampled data. I have so far only completed the slow implementation. As I work on the fast implementation, tests, statistical functions and documentation, I will tack on commits to this PR as i progress.