Skip to content

LombScargle: Use floating_mean=False for scipy implementation - #17163

Closed
pllim wants to merge 1 commit into
astropy:mainfrom
pllim:ls-tst-fail-hackfix
Closed

LombScargle: Use floating_mean=False for scipy implementation#17163
pllim wants to merge 1 commit into
astropy:mainfrom
pllim:ls-tst-fail-hackfix

Conversation

@pllim

@pllim pllim commented Oct 11, 2024

Copy link
Copy Markdown
Member

Description

This pull request is to make CI in devdep pass again due to scipy/scipy#21277, following suggestion in #17149 (comment) .

My main goal is to get rid of the failures so we can catch other failures during feature freeze frenzy, unless a better solution can be proposed very soon. Otherwise, I suggest we merge this first, and open follow-up issue to debate the actual science impact of upstream changes.

Fixes #17149

  • 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.

@pllim pllim added this to the v6.1.5 milestone Oct 11, 2024
@github-actions

github-actions Bot commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

👋 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?

@dhomeier

dhomeier commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

See scipy/scipy#21277 (comment) – the new default behaviour is just reproducing what has already been implemented as fit_mean=True for the other methods, so I think this should be added now to method='scipy' as well.

@neutrinoceros

neutrinoceros commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

Just a note that I'd like to get the opportunity to review and comment this over the weekend, so please do not merge right away. Thanks !

@jakevdp

jakevdp commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

Maybe not for this PR which is focused on fixing failing tests, but moving forward it might be worth implementing fit_mean by passing it to the new floating_mean argument of the scipy method, i.e. remove this warning if a suitably new scipy version is installed:

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

Lines 205 to 207 in fefc240

if method == "scipy":
if kwds.pop("fit_mean"):
raise ValueError("scipy method does not support fit_mean=True")

@pllim pllim mentioned this pull request Oct 11, 2024
Closed
@pllim

pllim commented Oct 11, 2024

Copy link
Copy Markdown
Member Author

Thanks for the suggestion, @jakevdp ! I have opened #17166 as a follow-up issue.

@pllim @dhomeier
Use floating_mean=False for scipy LS
986ca45 
in lombscargle_scipy to preserve backward compatibility.

Co-authored-by: Derek Homeier <[email protected]>
@pllim
pllim force-pushed the ls-tst-fail-hackfix branch from 6c30116 to 986ca45 Compare October 11, 2024 21:06
@pllim

pllim commented Oct 11, 2024

Copy link
Copy Markdown
Member Author

Re: #17163 (comment)

@neutrinoceros , no worries! I'll leave it as draft for now.

@pllim
pllim requested a review from neutrinoceros October 11, 2024 21:07
neutrinoceros
neutrinoceros reviewed Oct 12, 2024
View reviewed changes

@neutrinoceros neutrinoceros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I'm caught up now, and it seems that we're now close to a consensus for reverting the backward incompatible change upstream. See

So, the present PR might actually become unnecessary; however I'd argue we should still take this patch in because it adds clarity of intention on our side, and gives upstream more leeway if they want to change the default value of this argument in the future (preferably over a proper deprecation cycle).
That said, I'd recommend to focus on upstream discussions for now, and come back to this PR once we've all reached a consensus (which seems to be in sight).

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

# Note: scipy input accepts angular frequencies
p = signal.lombscargle(t, y, 2 * np.pi * frequency)
# floating_mean=True will fail tests due to https://github.com/scipy/scipy/pull/21277

@neutrinoceros neutrinoceros Oct 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this comment might become confusing and unnecessary if floating_mean=True never becomes the default, as is the direction it's taking upstream. We'll probably want to amend or simply remove it.

@neutrinoceros neutrinoceros mentioned this pull request Oct 12, 2024
Closed
@dhomeier dhomeier mentioned this pull request Oct 13, 2024
Closed
1 task
@pllim

pllim commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

As per #17170 (comment) , this is no longer needed.

@pllim pllim closed this Oct 14, 2024
@pllim
pllim deleted the ls-tst-fail-hackfix branch October 14, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhomeier dhomeier dhomeier left review comments
@neutrinoceros neutrinoceros neutrinoceros left review comments

Assignees

No one assigned

Labels

no-changelog-entry-needed timeseries

Projects

None yet

Milestone

v6.1.5

Development

Successfully merging this pull request may close these issues.

TST: test_lombscargle.py has 80 failed tests in devdeps

4 participants

@pllim @dhomeier @neutrinoceros @jakevdp