ENH: add support for `fit_mean=True` to `lombscargle_scipy` by neutrinoceros · Pull Request #17600 · astropy/astropy

neutrinoceros · 2025-01-03T16:26:40Z

Description

Scipy 1.15.0 was just released.
Fixes #17166
close #17170

opening as a draft for now because I see tests are failing at the time being but I'm running out of time for today so I'd rather have this in the open in case I forgot something obvious and someone else can point it out.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-01-03T16:27:05Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2025-01-03T16:27:07Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2025-01-03T16:30:49Z

+from astropy.utils import minversion
 from astropy.utils.compat.optional_deps import HAS_SCIPY

+SCIPY_LT_1_15 = not minversion("scipy", "1.15.0") if HAS_SCIPY else True


Instead of redefining it, can we import from the one in main.py?

neutrinoceros · 2025-01-06T10:49:13Z

I'm seeing 2 issues with my current approach:

I seems that test failures reveal that this implementation is more complicated than just passing an extra argument. I have close to no experience of the underlying equations and algorithms (I wasn't expecting that I would need to), so I can't really estimate how long it'd take me to get familiar enough to solve the problem myself. Maybe @jakevdp could provide some insight there.

Other implementations (lombscargle_chi2, lombscargle_cython ...) have fit_mean=True as the default value, but it's not possible to satisfy signature consistency and backward compatibility here, because lombscargle_scipy only supports fit_mean=False historically. I think this should be adressed through a deprecation cycle:

start with fit_mean=None as the default, and raise a deprecation warning if method="scipy" and fit_mean isn't provided (but stay backward compatible in that case: effectively use fit_mean=False as the default)
when we stop supporting scipy 1.14, we can end the deprecation cycle and change the default value to True

jakevdp · 2025-01-06T15:33:24Z

Thanks for opening this – I'm not sure what the numerical differences are between scipy's fit_mean and the one here.

because lombscargle_scipy only supports fit_mean=False historically. I think this should be adressed through a deprecation cycle:

lombscargle_scipy is a non-public API, so no deprecation cycle is necessary to make this change.

neutrinoceros · 2025-01-06T19:55:41Z

lombscargle_scipy is a non-public API, so no deprecation cycle is necessary to make this change.

You're right. However, the public API lombscargle has fit_mean=True as the default value, regardless of what's passed as method, so API would still naturally change in a backwards incompatible fashion if we were to do the naive thing. This is especially important because scipy can be selected automatically with method="auto". This should be clearer with the comments and refactors I just pushed.

jakevdp · 2025-01-06T21:56:22Z

I think the mismatched output is probably due to these normalization terms no longer being correct in the case of floating_mean=True:

astropy/astropy/timeseries/periodograms/lombscargle/implementations/scipy_impl.py

Lines 62 to 71 in 654e241

    
           if normalization == "psd": 
        
               pass 
        
           elif normalization == "standard": 
        
               p *= 2 / (t.size * np.mean(y**2)) 
        
           elif normalization == "log": 
        
               p = -np.log(1 - 2 * p / (t.size * np.mean(y**2))) 
        
           elif normalization == "model": 
        
               p /= 0.5 * t.size * np.mean(y**2) - p 
        
           else: 
        
               raise ValueError(f"normalization='{normalization}' not recognized")

They're computed in terms of chi-square values, and adding the floating mean changes the model being fit, and thus changes the chi square value.

In each of those expressions, I think you probably you need np.mean(y**2) for floating_mean=False, and np.mean((y - y.mean())**2) for floating_mean=True.

neutrinoceros · 2025-01-07T08:35:05Z

Thanks a lot, that did the trick !
now the only remaining issue is about evolving the API. In my next commit I'll flesh out my proposed solution.

neutrinoceros · 2025-01-07T09:34:36Z

This should be everything. Admittedly the situation is worse than I thought, but in hindsight, I'm even more convinced that we need to evolve the API for consistency.
At the moment, there is a conflict between default values in the LombScargle.power method, specifically

fit_mean=True
method='auto' might resolve to method='scipy', which, until this PR, doesn't support fit_mean=True

My proposed deprecation path leads to resolving this conflict in the somewhat distant future when we drop support for scipy<1.14, but isn't perfect, because default values are also used internally in LombScargle.false_alarm_probability, which I don't think we can evolve smoothly.

jakevdp · 2025-01-07T13:13:01Z

        frequency. This can lead to more accurate results, especially in the
        case of incomplete phase coverage.
+        If unspecified, power and autopower methods will use fit_mean=True in
+        most situations, except for method='scipy' where it is set to False.


How about "where it it set to False and raises a deprecation warning"

jakevdp · 2025-01-07T13:15:16Z

    center_data : bool, optional
        if True, pre-center the data by subtracting the weighted mean
        of the input data.
+    fit_mean : bool (default: False)


I think this should default to True like in other implementations. Since this is a non-public API, there is no problem with backward compatibility; we just need to make sure that the public APIs call this function in a backward-compatible way.

Good call, although it makes me wonder what's the point for default values in these private APIs: if they're used correctly, we should actually never get to use defaults, I think ?

jakevdp · 2025-01-07T13:17:32Z

    if center_data:
        y = y - y.mean()

+    if SCIPY_LT_1_15:


I think the fit_mean=True version incompatibility should be checked here:

if SCIPY_LT_1_15: if fit_mean: raise ValueError(...) kwargs = {} else: kwargs = {"floating_mean": fit_mean}

I usually try to keep raise statements for invalid user inputs as close to public API calls as possible to minimize confusion. I'm worried that seeing an internal function raise an exception could be interpreted as a bug. If you're unconvinced I'll go with your suggestion.

Maybe do both then?

To elaborate: lombscargle_scipy should raise a ValueError if the inputs are invalid rather than silently choosing different parameters, if only for the sake of helping internal refactors to work correctly.

If we don't want this ValueError to surface directly to the user, then we should also raise it higher in the stack.

If we go with both, we'll need a test for each, which seems excessive to me: warnings from the private function should be effectively unreachable

ping @jakevdp
this looks like the one unresolved point in your review

jakevdp · 2025-01-07T13:21:52Z

+        return p
+
+    if kwargs.get("floating_mean", False):
+        yoffset = y.mean()


This is unnecessary if center_data = True.

Rather than parameterizing via yoffset, I think we should parameterize by chi2_ref, and take into account center_data for efficiency. Maybe something like this?

if center_data or not fit_mean: chi2_ref = 0.5 * t.size * np.mean(y ** 2) else: chi2_ref = 0.5 * t.size * np.mean((y - y.mean()) ** 2)

jakevdp · 2025-01-07T13:23:58Z

    assume_regular_frequency=False,
    normalization="standard",
-    fit_mean=True,
+    fit_mean=None,


Can we leave this at fit_mean=True, and just raise a DeprecationWarning in cases where it is currently dropped? It seems strange to change the default if we're not changing behavior.

yes, this is redundant now anyway. Thanks for pointing it out !

jakevdp · 2025-01-07T13:25:51Z

    )
+    # Note that this condition may be simplified (remove `and not fit_mean`)
+    # once fit_mean=True is made the default value for method='scipy'
    prefer_scipy = "scipy" in methods and dy is None and not fit_mean


I think we should be able to do this now: e.g.

prefer_scipy = "scipy" in methods and dy is None and not (SCIPY_LT_1_15 and fit_mean)

So long as we handle fit_mean appropriately at the place where this is called.

good call. This makes the comment redundant and the simplification more obvious when the time comes !

neutrinoceros · 2025-01-07T13:58:59Z

@jakevdp thanks for your review. I've taken your suggestions into account, with one exception (see #17600 (comment)) let me know what you think, I'm happy to go with your suggestion if my argument doesn't win you over.

neutrinoceros · 2025-03-17T08:56:05Z

I am just not remembering that #17170 existed... so, this is technically an alternative to it, I'll link both PRs are mutually exclusive.

github-actions · 2025-06-07T05:43:24Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

astrofrog · 2026-09-16T08:32:28Z

Closing in favor of #20412 which cherry picks one of the commits from here related to normalization.

…g-mean model, where the reference chi-squared is the variance about the mean rather than about zero (extracted from astropy#17600)

github-actions Bot added the timeseries label Jan 3, 2025

pllim reviewed Jan 3, 2025

View reviewed changes

pllim added this to the v7.1.0 milestone Jan 3, 2025

ENH: add support for fit_mean=True to lombscargle_scipy

047ecce

neutrinoceros force-pushed the timeseries/feat/17166/support_fit_mean_lombscargle_scipy branch from 44af5e8 to 047ecce Compare January 4, 2025 10:27

neutrinoceros changed the title ~~ENH: add support for fit_mean=True to lombscargle_scipy~~ ENH: add support for fit_mean=True to lombscargle_scipy Jan 6, 2025

add docstring fragment

8a4ee66

differentiate fit_mean default values depending on method

e96957e

fixup impl

3ba10fb

add deprecation warning

8472759

neutrinoceros force-pushed the timeseries/feat/17166/support_fit_mean_lombscargle_scipy branch from cceedb0 to 2e63b95 Compare January 7, 2025 10:11

avoid self-triggered deprecation warnings

705a20e

neutrinoceros force-pushed the timeseries/feat/17166/support_fit_mean_lombscargle_scipy branch from 2e63b95 to 705a20e Compare January 7, 2025 10:19

jakevdp suggested changes Jan 7, 2025

View reviewed changes

neutrinoceros added 2 commits January 7, 2025 14:42

fixup! avoid self-triggered deprecation warnings

beed6ba

fixup! fixup! avoid self-triggered deprecation warnings

8d65502

fixup! fixup! fixup! avoid self-triggered deprecation warnings

39240a4

neutrinoceros mentioned this pull request Mar 17, 2025

Enable fit_mean=True in lombscargle_scipy for Scipy 1.15+ #17170

Closed

1 task

astrofrog modified the milestones: v7.1.0, v7.2.0 Apr 28, 2025

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Jun 7, 2025

neutrinoceros added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Jun 7, 2025

astrofrog modified the milestones: v7.2.0, v8.0.0 Nov 3, 2025

astrofrog modified the milestones: v8.0.0, v8.1.0 May 5, 2026

astrofrog mentioned this pull request Sep 15, 2026

Enable fit_mean=True for Lomb-Scargle periodograms #20412

Merged

2 tasks done

astrofrog removed the keep-open label Sep 16, 2026

astrofrog closed this Sep 16, 2026

Conversation

neutrinoceros commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Jan 3, 2025

Uh oh!

github-actions Bot commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jan 6, 2025

Uh oh!

jakevdp commented Jan 6, 2025

Uh oh!

neutrinoceros commented Jan 6, 2025

Uh oh!

jakevdp commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Jan 7, 2025

Uh oh!

neutrinoceros commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakevdp Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakevdp Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jan 7, 2025

Uh oh!

neutrinoceros commented Mar 17, 2025

Uh oh!

github-actions Bot commented Jun 7, 2025

Uh oh!

astrofrog commented Sep 16, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neutrinoceros commented Jan 3, 2025 •

edited

Loading

jakevdp commented Jan 6, 2025 •

edited

Loading

neutrinoceros commented Jan 7, 2025 •

edited

Loading

jakevdp Jan 7, 2025 •

edited

Loading

jakevdp Jan 7, 2025 •

edited

Loading