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

Swap lightkurve periodogram for astropy method in Ephemeris #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Feb 7, 2025

If you load data into LCviz with sampling varying from minutes apart to decades apart, the lightkurve.periodogram functionality chokes. It chokes because there are some assumptions that lightkurve makes that cause issues here. The main assumption is about time sampling. The time difference between the nearest measurements in this series is of order minutes, but the full span is decades. Sampling evenly at the nyquist frequency gives a tremendous array
and by default, lightkurve uses an O(N^2) implementation of the L-S periodogram. One very simple solution is to not make the same assumptions as lightkurve does.

Ephemeris calls this on the data:

per = periodogram.LombScarglePeriodogram.from_lightcurve(self.dataset.selected_obj)

which is where the problems start.

We can fix that up like this:

lc = self.dataset.selected_obj
ls = LombScargle(lc.time, lc.flux, lc.flux_err, normalization='psd')
freq = ls.autofrequency()
power = ls.power(freq)
self.period_at_max_power = (1 / freq[np.argmax(power)]).value

just using the astropy L-S implementation directly without going through lightkurve overcomes the frequency array problem.

@bmorris3 bmorris3 added this to the 1.0.1 milestone Feb 7, 2025
@bmorris3 bmorris3 added the bug Something isn't working label Feb 7, 2025
@bmorris3 bmorris3 requested a review from kecnry February 7, 2025 16:05
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.14%. Comparing base (7b78df5) to head (0475a84).
Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
- Coverage   93.92%   90.14%   -3.79%     
==========================================
  Files          37       41       +4     
  Lines        1598     2192     +594     
==========================================
+ Hits         1501     1976     +475     
- Misses         97      216     +119     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything here that can be ported upstream so that we can eventually revert and use lightkurve directly?

We also should probably update the plugin docs which currently state that this just wraps lightkurve methods.

(Also needs a changelog entry - dev test failures are unrelated)

@@ -562,13 +563,26 @@ def _update_periodogram(self, *args):
if self.method == 'Box Least Squares':
try:
per = periodogram.BoxLeastSquaresPeriodogram.from_lightcurve(self.dataset.selected_obj) # noqa
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is BLS not subject to the same issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants