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

Add Lomb-Scargle detection function #1070

Merged
merged 9 commits into from Sep 7, 2017

Conversation

Projects
None yet
2 participants
@wegenmat
Contributor

wegenmat commented Jun 20, 2017

This pull request adds the Lomb-Scargle algorithm for period detection to gammapy/time. The Lomb-Scargle is from astropy. It is extended by different significance criteria for periodogram peaks. The spectral window function is introduced to investigate the influence of sampling on the periodogram.

Matthias Wegen

@cdeil cdeil self-assigned this Jun 20, 2017

@cdeil cdeil added the feature label Jun 20, 2017

@cdeil cdeil added this to the 0.7 milestone Jun 20, 2017

@cdeil

@wegenmat - Thanks!

I've done a first round of review and left inline comments. After we've iterated a bit on this and polished the code / tests / docs, I would then ask someone else to review the API / method.

Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle_gammapy.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle_gammapy.py Outdated
@wegenmat

This comment has been minimized.

Show comment
Hide comment
@wegenmat

wegenmat Jun 20, 2017

Contributor

I did some of the recommend minor changes. I am not quite sure about the plotting function. On the one hand, it is not essential for the periodogram, but one the other hand, since at least the spectral window function needs to be inspected by eyesight, plotting comes in handy for the analysis.

Contributor

wegenmat commented Jun 20, 2017

I did some of the recommend minor changes. I am not quite sure about the plotting function. On the one hand, it is not essential for the periodogram, but one the other hand, since at least the spectral window function needs to be inspected by eyesight, plotting comes in handy for the analysis.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 20, 2017

Member

I did some of the recommend minor changes. I am not quite sure about the plotting function. On the one hand, it is not essential for the periodogram, but one the other hand, since at least the spectral window function needs to be inspected by eyesight, plotting comes in handy for the analysis.

OK to keep the plotting function. But then you have to write a good docstring explaining what the inputs are and what the plots show.

Member

cdeil commented Jun 20, 2017

I did some of the recommend minor changes. I am not quite sure about the plotting function. On the one hand, it is not essential for the periodogram, but one the other hand, since at least the spectral window function needs to be inspected by eyesight, plotting comes in handy for the analysis.

OK to keep the plotting function. But then you have to write a good docstring explaining what the inputs are and what the plots show.

Show outdated Hide outdated docs/time/period.rst Outdated
@cdeil

@wegenmat - I left a few more inline comments with suggestions. Let me know if you have any questions or if I should look again at something by leaving a comment.

Show outdated Hide outdated gammapy/time/lombscargle.py Outdated
Show outdated Hide outdated gammapy/time/lombscargle.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lombscargle.py Outdated

@cdeil cdeil changed the title from Lomb-Scargle for gammapy.time to Add Lomb-Scargle detection function Jun 20, 2017

wegenmat and others added some commits Jun 23, 2017

Matthias Wegen
improved doc strings and inline comments in lomb_scargle.py and lomb_…
…scargle_plot.py, doc for period detection period.rst is still to be done
@wegenmat

This comment has been minimized.

Show comment
Hide comment
@wegenmat

wegenmat Jun 26, 2017

Contributor

A new idea about the plotting function: since it is the same for the Lomb-Scargle and the robust regression techniques (upcoming), it may be smart to outsource it to an independent method that is dedicated to period detection in general, not only Lomb-Scargle. Your thoughts about this, @cdeil ?

Contributor

wegenmat commented Jun 26, 2017

A new idea about the plotting function: since it is the same for the Lomb-Scargle and the robust regression techniques (upcoming), it may be smart to outsource it to an independent method that is dedicated to period detection in general, not only Lomb-Scargle. Your thoughts about this, @cdeil ?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 26, 2017

Member

Sure, if the plotting function can be applied more generally, it's a good idea to make it work for both cases!

Member

cdeil commented Jun 26, 2017

Sure, if the plotting function can be applied more generally, it's a good idea to make it work for both cases!

Matthias Wegen
added plot_periodogram as indepentent method, completed doc string fo…
…r plot_periodogram and lomb_scargle, added doc string to test
@wegenmat

This comment has been minimized.

Show comment
Hide comment
@wegenmat

wegenmat Jun 26, 2017

Contributor

ready for reviewing

Contributor

wegenmat commented Jun 26, 2017

ready for reviewing

@cdeil

@wegenmat - I left a bunch of inline comments. The most important one is the request to add a working example to the high-level docs. Having that will make it much more likely that people will use this, and also easier to do the review if there's a working example one can easily start playing with.

Show outdated Hide outdated docs/time/period.rst Outdated
Show outdated Hide outdated docs/time/period.rst Outdated
Show outdated Hide outdated docs/time/period.rst Outdated
Show outdated Hide outdated gammapy/time/lomb_scargle.py Outdated
Show outdated Hide outdated gammapy/time/lomb_scargle.py Outdated
Show outdated Hide outdated gammapy/time/lomb_scargle.py Outdated
Show outdated Hide outdated gammapy/time/lomb_scargle.py Outdated
Show outdated Hide outdated gammapy/time/lomb_scargle.py Outdated
Show outdated Hide outdated gammapy/time/plot_periodogram.py Outdated
Show outdated Hide outdated gammapy/time/tests/test_lomb_scargle.py Outdated

wegenmat and others added some commits Jun 26, 2017

Matthias Wegen
@cdeil

cdeil approved these changes Sep 7, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 7, 2017

Member

@wegenmat - Thanks for all your work on this!

There's several more points to discuss here, but I'm merging this now into Gammapy master because it's easier to continue via commits there or in follow-up pull requests.

Member

cdeil commented Sep 7, 2017

@wegenmat - Thanks for all your work on this!

There's several more points to discuss here, but I'm merging this now into Gammapy master because it's easier to continue via commits there or in follow-up pull requests.

@cdeil cdeil merged commit dd38597 into gammapy:master Sep 7, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 7, 2017

Member

@wegenmat - I've done some code clean-up in f2cd0cc and added you to the Gammapy contributor list in 2affbe7

Please review the changes in f2cd0cc and let me know if you have any questions or disagree with something. We've found that setting plotting style in library functions is not a good pattern because styles keep changing (e.g. matplotlib 1.0 vs 2.0) and it's best to leave styling to users.

Some questions / comments:

  • Is the implementation of _cvm correct? You have a for loop and a variable sumbeta, but then you don't actually accumulate in that variable, which in effect will lead to the values from just the last iteration being used (and the for loop not being needed). Looks like a bug, no?
  • The tests are very slow. Can you choose a faster test-case? Does it really help to run two test cases for the plotting function or would you test just as much by just running a single test case to execute it once?
  • The link to the reference to Süveges is super long. Is that reference listed in ADS? If yes, could you please link there instead?

@wegenmat - Maybe you could open a new follow-up PR (starting from master) where we can do further improvements for this code?

--

The docs build is currently failing because you're referencing PNG images in gammapy-extra that aren't there yet. Please make a pull request against gammapy-extra to add them.

Member

cdeil commented Sep 7, 2017

@wegenmat - I've done some code clean-up in f2cd0cc and added you to the Gammapy contributor list in 2affbe7

Please review the changes in f2cd0cc and let me know if you have any questions or disagree with something. We've found that setting plotting style in library functions is not a good pattern because styles keep changing (e.g. matplotlib 1.0 vs 2.0) and it's best to leave styling to users.

Some questions / comments:

  • Is the implementation of _cvm correct? You have a for loop and a variable sumbeta, but then you don't actually accumulate in that variable, which in effect will lead to the values from just the last iteration being used (and the for loop not being needed). Looks like a bug, no?
  • The tests are very slow. Can you choose a faster test-case? Does it really help to run two test cases for the plotting function or would you test just as much by just running a single test case to execute it once?
  • The link to the reference to Süveges is super long. Is that reference listed in ADS? If yes, could you please link there instead?

@wegenmat - Maybe you could open a new follow-up PR (starting from master) where we can do further improvements for this code?

--

The docs build is currently failing because you're referencing PNG images in gammapy-extra that aren't there yet. Please make a pull request against gammapy-extra to add them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment