-
Notifications
You must be signed in to change notification settings - Fork 151
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
Phase dispersion minimization in stingray! #716
Conversation
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.
Hey @dhuppenkothen, great to have another tool for periodicities!
I see from the new docs that you created a notebook to show off the method's performance, but it's currently missing. Would you mind adding it to the notebooks repository as a separate PR?
Also, there must have been some issue with black and a lot of the diff is white space and unrelated changes. Would you mind rebasing and/or re-opening based on the most recent main?
Thanks!
bd38497
to
4d7ddcf
Compare
We'd need to merge the notebook PR first, so that we can update the submodule before merging this PR. |
See #721! |
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.
A few comments here and there!
Parameters | ||
---------- | ||
times : array of floats | ||
Photon arrival times, or, if `weights` is set, |
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.
What happens if weights
is set? Don't leave us hanging!
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 do like a good cliffhanger! :D
I think I introduced some bugs while I fixed merge conflicts when I merged the current main branch. Should be fixed now.
stingray/tests/test_stats.py
Outdated
@@ -33,6 +33,19 @@ def test_fold_detection_level(): | |||
) | |||
|
|||
|
|||
def test_fold_detection_level(): |
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.
Duplicate name?
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.
Yup, should be test_pdm_detection_level
# dummy array for the error, which we don't have for the variance | ||
raw_profile_err = np.zeros_like(raw_profile) | ||
|
||
else: |
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 these lines are not tested
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.
Added tests to test_pulse.py
stingray/pulse/pulsar.py
Outdated
expocorr : bool, default False | ||
Correct each bin for exposure (use when the period of the pulsar is | ||
comparable to that of GTIs) | ||
|
||
mode : str, ["ef", "pdm"], default "pdm" |
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.
Why is pdm
the default?
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.
It is not in the code, but apparently I hallucinated while writing the doctoring. :D
Accidentally deleted the new PDM stats functions from |
Codecov Report
@@ Coverage Diff @@
## main #716 +/- ##
==========================================
+ Coverage 97.20% 97.22% +0.01%
==========================================
Files 42 42
Lines 7805 7848 +43
==========================================
+ Hits 7587 7630 +43
Misses 218 218
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Phase dispersion minimisation is a close cousin to Epoch Folding, so I piggy-backed off the existing infrastructure and implemented it in stingray. Mostly used for asteroids and binary stars rather than pulsars, but since all the other methodology is in the
pulse
sub-package, that's where it might live for the moment.