Enable `fit_mean=True` in `lombscargle_scipy` for Scipy 1.15+ by dhomeier · Pull Request #17170 · astropy/astropy

dhomeier · 2024-10-13T17:58:20Z

Description

Replacement of #17163, that will enable the scipy lombscargle implementation to be called with fit_mean=True for scipy versions supporting this as the floating_mean option introduced with scipy/scipy#21277.
This is also unskipping the respective tests for the scipy method, plus a number of others with constant uncertainties that are actually accepted by this method (though not used for anything, which however seems to only be relevant for the psd normalisation).

The discussion over the final behaviour, and in particular defaults, of the floating_mean option was still open in scipy/scipy#21696, but has been settled for reverting to the old default as of scipy/scipy#21697.
I'd still note some points:

This PR is not changing any defaults for fit_mean, as its default has always been True in Lombscargle, and the scipy method can currently only be used when explicitly setting fit_mean=False:

>>> lm = LombScargle(t, y, None)
>>> lm.power(frequencies, method='scipy')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "astropy/timeseries/periodograms/lombscargle/core.py", line 381, in power
    power = lombscargle(
  File "astropy/timeseries/periodograms/lombscargle/implementations/main.py", line 209, in lombscargle
    raise ValueError("scipy method does not support fit_mean=True")
ValueError: scipy method does not support fit_mean=True

The eventual default behaviour of floating_mean will not matter for this implementation, since it always explicitly sets it to the value of fit_mean.
The only default that is changed at all is, that the default method for fit_mean=True will now often become scipy, where it has usually been cython before. This could even be changed by not making this change in prefer_scipy, but since the results are identical, and performance per the tests on the scipy side should not be any worse than the Cython version, I would consider this an irrelevant change on the user side.
One more thing I noted is that all other methods seem to handle fit_mean and center_data in the same branch, i.e effectively the latter is always treated as True if the former is. This is obviously related to the somewhat misleading documentation described in Clarify interaction between center_data and fit_mean in LombScargle #17038. I think this is equivalent to the old precenter option in signal.lombscargle, but anyway scipy handles both independently, and will thus return different results for fit_mean=True, center_data=False. For consistency I am therefore also forcing the latter to True in this case, but the wrapper function itself still accepts both combinations. I think this still needs more discussion which combinations should be supported or make sense at all.

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 · 2024-10-13T17:58:45Z

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.

jakevdp · 2024-10-13T18:21:05Z

+        if fit_mean and SCIPY_LT_1_15:
+            pytest.skip("SciPy 1.15+ required for using `fit_mean=True`")
+        elif (errors == "full" or errors == "partial" and normalization == "psd"):
+            pytest.skip("scipy method only supports uniform uncertainties dy")


This is no longer true in scipy 1.15 – should we plumb through nonuniform uncertainties for scipy 1.15 as well?

That would certainly be worth a stab; gonna look into this and see if there is a chance to get it in before feature-freeze as well.

Yes, passing w / w.sum() as in the other methods as weights seems to work, just the psd normalisation will need some more investigation.

jakevdp · 2024-10-13T18:23:51Z

        assume_regular_frequency or _is_regular(frequency)
    )
-    prefer_scipy = "scipy" in methods and dy is None and not fit_mean
+    prefer_scipy = "scipy" in methods and dy is None and not (fit_mean and SCIPY_LT_1_15)


This logic is off I think – e.g. if fit_mean=True and scipy is version 1.14, this will return True, and then further down this will lead to an error because scipy 1.14 does not support fit_mean=True.

But not () is False, so this is correctly skipped with 1.14 (those tests are passing, but I think I need to rebase to get rid of the failure in modeling).

dhomeier · 2024-10-13T19:09:44Z

Fixed the changelog locally, but other failures are unrelated, so holding off pushing again until that is resolved.

tylerjereddy · 2024-10-13T22:16:01Z

Latest SciPy main branch just switched back to floating_mean=False default.

neutrinoceros · 2024-10-14T08:51:29Z

@dhomeier do you want my review now or after the be PR is undrafted ?

jakevdp · 2024-10-14T13:07:19Z

Since the scipy change is still settling upstream, I'd recommend waiting on this PR until things stabilize.

dhomeier · 2024-10-14T13:20:05Z

Comments are always welcome, but I'll continue looking into adding the support for non-uniform dy to the scipy method.
But since scipy/scipy#21697 is merged, I am marking this ready now, as the uncertainties could also be handled in a follow-up.

jakevdp · 2024-10-14T13:26:44Z

+    if method == "scipy":
+        P_expected = ls.power(frequency, method="cython")
+    else:
+        P_expected = ls.power(frequency)


This change seems strange to me – the default is scipy in some cases, but not in others. If it's important to do this for scipy, why is it not important to do this in other cases?

I did not have a full overview of the defaults, but it seems for this dataset (not prefer_fast) the only other default would be cython, so using slow in that case would probably make sense. I guess in principle the comparisons should be symmetric, but when debugging the center_data settings I was not so sure. Anyway would make more sense than to compare a method to itself imo.

Sure, but you're still comparing a method to itself whenever the default does not happen to be "scipy" – if that's a problem, shouldn't you fix it in all cases? And if it's not a problem, then why change this code at all?

No, only for the cython case I'd add – or are there any configurations I have overlooked when auto would not choose either cython or scipy? Anyway that was the behaviour before, so just leaving it that way does not seem useful. I guess we could also always use slow for reference anytime method != "slow", just worried that might slow down the tests needlessly. ;-)

For what it's worth, my reasoning in the original version of this was that doing the simple thing does not result in any lack of test coverage when running the full test suite, and also serves to test that the default is doing something sensible, even in PRs like this where the default has new behavior.

I think it would be cleanest to revert this change.

Basically this was trying to take up @mhvk's suggestion to test the 'scipy' method against one of the internals, which was not (explicitly) done before. The issues I also have with the existing construct are

~¼ of the tests is not really comparing anything than repeating the same function call.

'scipy', and for some cases 'cython', is only ever tested indirectly (when some other methods are failing against it as the reference), which is creating confusing diagnostics in case that 'scipy' is broken – e.g. in TST: test_lombscargle.py has 80 failed tests in devdeps #17149 it initially appeared that all other methods were broken until someone remembered that 'scipy' was used as the baseline for those tests.

OK, then if (1) is a problem, I think we should solve it for all test runs, rather than just for some. For (2), I think it's not going to be possible to escape this entirely, because some method needs to be chosen as a baseline for any particular test, and that baseline may someday break.

pllim · 2024-10-14T20:10:16Z

waiting on this PR until things stabilize

Should we move this to draft and remilestone to v7.1 then?

Thanks, all!

dhomeier · 2024-10-14T22:09:28Z

Should we move this to draft and remilestone to v7.1 then?

scipy PR is merged, and did not affect how we are calling signal.lombscargle here anyway.

I have set a clearer precedence for the baseline P_expected in the test now, so it will also show what method is compared against.

I have implemented a first version for nonuniform weights in lombscargle_scipy, but setting them to dy**-2 as in the other methods results in larger discrepancies (up to a few %) in the returned P. The peaks still seem to be identified correctly, so one could loosen the testing tolerances in those cases, but obviously there are some subtle differences in the way the weights are applied that I could not track down yet in the scipy code.
So I could push those changes here for discussion, or propose this PR completed and open another one for the array-dy.

jakevdp · 2024-10-14T22:41:34Z

scipy PR is merged, and did not affect how we are calling signal.lombscargle here anyway.

This is true so long as long as we're certain that scipy is not going to rethink argument names or default behavior again before the next release.

neutrinoceros · 2024-10-15T09:27:51Z

To supplement @jakevdp, and as I already said on the issue (#17166 (comment)), I don't think we should merge this before scipy 1.15 is actually released (or has a release candidate, at least): however unlikely it is that the feature is changed again or removed before the actual release, they might want/need to, and having this patch merged already could create a catastrophic situation on our side (especially if released), so I strongly feel this shouldn't be on the 7.0 milestone.

dhomeier · 2025-03-14T17:09:18Z

Rebased to have the tests run against Scipy 15.2 now that v15 API is fixed.
Will look into the open issues about more support for non-uniform weights dy with Scipy.

tylerjereddy · 2025-03-14T17:24:14Z

If you need something from SciPy in the next 1.15.x series release (or for 1.16.x in the spring), please open an issue in SciPy repo and ping me. Maybe you don't need anything, but I just saw the notification and wanted to make sure.

neutrinoceros · 2025-03-17T09:01:50Z

Sorry I'm just now remembering this PR existed and pre-dated #17600, so I linked both PRs as mutually exclusive. However, they are not identical: my approach also includes deprecation cycles to eventually re-unify currently inconsistent default values for the fit_mean argument across different methods. I do believe that we should aim for maximal API consistency in the long run, for the sake of both users and maintainers, however, we do not need to use my PR for that, and I'd be perfectly happy to go with this one instead if my suggestion was adopted here. Sorry for not connecting the dots earlier.

github-actions · 2025-08-12T05:40:36Z

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.

adammj · 2025-08-18T04:08:40Z

I noticed that this hasn't been committed, and it references the scipy lombscargle parameter precenter. I opened up a PR to deprecate the parameter (scipy/scipy#23476). Please feel free to comment.

pllim · 2025-08-18T18:08:37Z

@dhomeier are you interested to revisit? Thanks, all!

dhomeier · 2025-08-18T18:16:11Z

This has slipped under the radar for some time, don't know why tests are currently hanging, but need to check what has materialised on the scipy side now first.

pllim · 2025-08-18T18:17:41Z

The CI is probably because you need to rebase.

dhomeier · 2025-08-18T18:21:19Z

But why are tests running at all when this has been inactive for 4 months? No point in rebasing right now, as one kwarg used here is marked to be deprecated by Scipy, so should try to find a solution without it.

pllim · 2025-08-18T21:39:02Z

But why are tests running at all when this has been inactive for 4 months?

Not sure what you mean. We bumped CI dependency versions, so the required job names changed from when you last ran this. Required jobs are always listed in status checks.

should try to find a solution without it.

Would it be cleaner to open a new PR at this point?

dhomeier · 2025-08-18T22:15:48Z

But why are tests running at all when this has been inactive for 4 months?

Not sure what you mean. We bumped CI dependency versions, so the required job names changed from when you last ran this. Required jobs are always listed in status checks.

This. So these 3 were added in main since last rebase? The way GH displays this is at least, well, unintuitive...

Would it be cleaner to open a new PR at this point?

Will decide when I have tried a rebase, and checked what is the status at Scipy (1.16 by now), and also compared to #17600.

pllim · 2025-08-18T22:21:47Z

So these 3 were added in main since last rebase?

More like we changed the job name when we bump Python and/or Matplotlib minversions.

neutrinoceros · 2025-09-10T09:11:52Z

I also expect CI to behave cleanly on a rebase. The jobs you see in your screenshot are now awaited specifically because they never ran, but are currently mandated for merging.

astrofrog · 2026-09-16T08:31:56Z

Closing in favor of #20412, which incorporates the commits from here

dhomeier requested review from jakevdp, neutrinoceros and pllim October 13, 2024 17:58

github-actions Bot added the timeseries label Oct 13, 2024

dhomeier marked this pull request as draft October 13, 2024 17:59

dhomeier added Bug Enhancement labels Oct 13, 2024

jakevdp reviewed Oct 13, 2024

View reviewed changes

dhomeier added this to the v7.0.0 milestone Oct 13, 2024

dhomeier force-pushed the scipy-fit-mean branch from 4073b17 to 90d7d0c Compare October 13, 2024 18:49

dhomeier force-pushed the scipy-fit-mean branch from 90d7d0c to 7a54238 Compare October 14, 2024 13:15

dhomeier marked this pull request as ready for review October 14, 2024 13:15

jakevdp reviewed Oct 14, 2024

View reviewed changes

This was referenced Oct 14, 2024

LombScargle: Use floating_mean=False for scipy implementation #17163

Closed

TST: test_lombscargle.py has 80 failed tests in devdeps #17149

Closed

dhomeier force-pushed the scipy-fit-mean branch 2 times, most recently from 9c0365a to e4f6ba9 Compare October 14, 2024 21:49

dhomeier added testing and removed Bug labels Oct 14, 2024

neutrinoceros removed this from the v7.0.0 milestone Oct 15, 2024

RUF021 fix

a095920

dhomeier removed the Close? Tell stale bot that this issue/PR is stale label Mar 14, 2025

neutrinoceros mentioned this pull request Mar 17, 2025

ENH: add support for fit_mean=True to lombscargle_scipy #17600

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 Aug 12, 2025

adammj mentioned this pull request Aug 18, 2025

DEP: signal.lombscargle: deprecate argument precenter scipy/scipy#23476

Merged

dhomeier removed the Close? Tell stale bot that this issue/PR is stale label Aug 18, 2025

dhomeier added the keep-open label Aug 18, 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

dhomeier commented Oct 13, 2024 • edited by neutrinoceros Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Oct 13, 2024

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!

dhomeier commented Oct 13, 2024

Uh oh!

tylerjereddy commented Oct 13, 2024

Uh oh!

neutrinoceros commented Oct 14, 2024

Uh oh!

jakevdp commented Oct 14, 2024

Uh oh!

dhomeier commented Oct 14, 2024 • 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!

jakevdp Oct 14, 2024 • 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!

dhomeier Oct 14, 2024 • 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!

jakevdp Oct 14, 2024 • 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!

jakevdp Oct 14, 2024 • 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!

pllim commented Oct 14, 2024

Uh oh!

dhomeier commented Oct 14, 2024

Uh oh!

jakevdp commented Oct 14, 2024

Uh oh!

neutrinoceros commented Oct 15, 2024

Uh oh!

dhomeier commented Mar 14, 2025

Uh oh!

tylerjereddy commented Mar 14, 2025

Uh oh!

neutrinoceros commented Mar 17, 2025

Uh oh!

github-actions Bot commented Aug 12, 2025

Uh oh!

adammj commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Aug 18, 2025

Uh oh!

dhomeier commented Aug 18, 2025

Uh oh!

pllim commented Aug 18, 2025

Uh oh!

dhomeier commented Aug 18, 2025

Uh oh!

pllim commented Aug 18, 2025

Uh oh!

dhomeier commented Aug 18, 2025

dhomeier commented Oct 13, 2024 •

edited by neutrinoceros

Loading

dhomeier commented Oct 14, 2024 •

edited

Loading

jakevdp Oct 14, 2024 •

edited

Loading

dhomeier Oct 14, 2024 •

edited

Loading

jakevdp Oct 14, 2024 •

edited

Loading

jakevdp Oct 14, 2024 •

edited

Loading

adammj commented Aug 18, 2025 •

edited

Loading