Skip to content
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

Baluev method plus an notebook example #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nhuntwalker
Copy link

Added Baluev 2008 method for calculating false alarm probabilities to lomb_scargle.py. Also added an ipynb for demonstrating the comparison between Baluev 2008 theoretical model and data from Gaussian white noise in the examples directory

@jakevdp
Copy link
Member

jakevdp commented Sep 15, 2015

Looks good. A couple things:

  • this is only valid for single-term lomb-scarlge, yes? In that case, this implementation should probably be in LombScargle rather than the base PeriodicModeler, where it would be inherited by other modelers for which the assumptions don't hold. I would probably add a NotImplemented method in the base class, and specialize it with the Baluev method in the LombScargle class.
  • we need some unit tests for the functionality. Probably just compute what the result should be for a known input, and confirm that the output matches (in case of future refactorings)

Thanks for putting this together!

@jakevdp
Copy link
Member

jakevdp commented Oct 19, 2015

Looks great, Nick! It looks like something has been changed in master, and there's now a merge conflict here. Can you merge your branch with master and fix the conflict?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants