TST: test_lombscargle.py has 80 failed tests in devdeps

Example log: https://github.com/astropy/astropy/actions/runs/11256675139/job/31299150063

This should be resolved before we cut v7.0 RC.

I think it is just one test running a lot of variants. One of the failures:

FAILED astropy/timeseries/periodograms/lombscargle/tests/test_lombscargle.py::
    test_all_methods[model-False-none-False-False-fastchi2] - AssertionError: 
Not equal to tolerance rtol=1e-07, atol=0

Mismatched elements: 40 / 40 (100%)
Max absolute difference among violations: 0.05446034
Max relative difference among violations: 4.22703988
 ACTUAL: array([0.000273, 0.001192, 0.003183, 0.00476 , 0.005066, 0.003902,
       0.002159, 0.001589, 0.001829, 0.001277, 0.000414, 0.000883,
       0.003696, 0.007136, 0.006279, 0.001731, 0.002283, 0.015811,...
 DESIRED: array([0.008445, 0.006723, 0.004143, 0.001797, 0.000969, 0.001431,
       0.002082, 0.002242, 0.002408, 0.002709, 0.00248 , 0.002272,
       0.003062, 0.004837, 0.006965, 0.007883, 0.006677, 0.005468,...
==== 80 failed, 28405 passed, 806 skipped, 235 xfailed in 362.91s (0:06:02) ====

astropy/astropy/timeseries/periodograms/lombscargle/tests/test_lombscargle.py

Lines 61 to 69 in a7e050b

    
           @pytest.mark.parametrize("method", ALL_METHODS_NO_AUTO) 
        
           @pytest.mark.parametrize("center_data", [True, False]) 
        
           @pytest.mark.parametrize("fit_mean", [True, False]) 
        
           @pytest.mark.parametrize("errors", ["none", "partial", "full"]) 
        
           @pytest.mark.parametrize("with_units", [True, False]) 
        
           @pytest.mark.parametrize("normalization", NORMALIZATIONS) 
        
           def test_all_methods( 
        
               data, method, center_data, fit_mean, errors, with_units, normalization 
        
           ):

Since the test does not skip for missing "optional" dependencies, the main suspect is numpy-dev changes.

I'm game to dig into this tommorrow unless someone wants to beat me to it (but of course, feel free to !).

I couldn't resist digging down a bit tonight and found the culprit with git bisect: scipy/scipy#21277
Since this change is advertised as "backward compatible" we'll need to clarify whether it's meant to be exactly equivalent (in which case there's a regression) or if it's expected to produce slightly different results, and some tolerance tweaking is needed on our side. I'll re-kindle the discussion upstream

To clarify my point from the upstream PR, the formulas are "exactly equivalent" mathematically (given all of the assumptions each make, etc.). However, mathematically exact does not mean numerically exact.

@neutrinoceros

Just looking at one number, 0.000273 turning into 0.008445 is indeed not numerically exact, so whoever patches this here needs to understand why the numbers have changed and whether it is okay to just slap on a scipy version check and using different truths based on that, or not.

Thanks for digging, @neutrinoceros!

@jakevdp

Let me ping @jakevdp since he implemented the lombscargle module. It may well be that the changes are within the noise (depending on how large the actual peak is), but would be good to be sure. Since there are multiple methods to calculate the Lomb Scargle periodogramme, presumably we can also run internal cross checks.

Tsk tsk, we have 2 weeks till feature freeze. I hope we have a path forward soon. 🤞

Wouldn't wanna pin a max version of scipy.

One of the lomb scargle methods just calls out to scipy, and scipy recently updated their implementation: scipy/scipy#21277. The old scipy lomb-scargle had some abnormal conventions if I recall correctly, and I suspect they fixed these which would lead to astropy test failures.

@jakevdp

@jakevdp are you available/interested in digging further yourself ? otherwise I might be able to help (from a technical standpoint ! this code is pretty foreign to me so I don't think I want to be our first choice here).
@pllim I'm not too worried about the upcoming feature freeze, this (from afar) looks like it could be resolved on our side as a bug fix ? maybe I'm being overly optimistic.

Re: bug fix -- Maybe true but not nice to have RC with failed job because we rely on users to test the RC on whatever envs they feel like (within reason).

@jakevdp

@jakevdp - could one test the scipy implementation against one of the ones we have ourselves?

@jakevdp

@jakevdp - could one test the scipy implementation against one of the ones we have ourselves?

That's essentially what the current tests do: the expected value is computed with the default method:

astropy/astropy/timeseries/periodograms/lombscargle/tests/test_lombscargle.py

Line 100 in a7e050b

P_expected = ls.power(frequency)

and this is compared against the value computed with the method being tested:

astropy/astropy/timeseries/periodograms/lombscargle/tests/test_lombscargle.py

Line 105 in a7e050b

P_method = ls.power(frequency, method=method, **kwds)

In this case I suspect the failures are for method='scipy', which uses scipy.stats.lombscargle to compute the result.

Does that then suggest that the new scipy method is, in fact, bad? (Sorry, no time to look into the details, but if scipy has become inconsistent with our methods, then that is something they should now - perhaps the changes need to be reverted before they are included in a release...)

It's not just method=scipy cases that are breaking, but these tests pass if scipy isn't installed at all

It's not just method=scipy cases that are breaking, but these tests pass if scipy isn't installed at all

scipy is used as the default method in some cases when it's available:

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

Line 87 in a7e050b

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

So failures where method != 'scipy' are probably cases where the baseline is scipy.

All of this so far is consistent with scipy's lombscargle having changed its numerics in scipy/scipy#21277

All of this so far is consistent with scipy's lombscargle having changed its numerics in scipy/scipy#21277

Yes, I already identified this PR as the breaking change earlier in this thread.

Just looking at one number, 0.000273 turning into 0.008445 is indeed not numerically exact, so whoever patches this here needs to understand why the numbers have changed and whether it is okay to just slap on a scipy version check and using different truths based on that, or not.

The really large relative errors > 4 all occur for the center_data=False, else they are always around 0.025, 0.06, 0.13 for standard, log and psd normalisations, respectively. This might indicate something related to the now "correct removal of y offsets" mentioned in the PR.
In fact it seems even more directly linked to the new floating_mean argument which does change the results significantly when there is a large systematic offset (as in our test data). Changing the call to

p = signal.lombscargle(t, y, 2 * np.pi * frequency, floating_mean=False)

actually already lets all the tests pass again (as it was supposed, fully restoring the previous behaviour).
So that might be a quick fix, but I think we should still go through the discussion and literature and find out if our old results really are the desired or mathematically sound ones.

@jakevdp

I've been using the floating-mean version of LS for over a decade, and @jakevdp 's 2018 paper (which I found while doing the scipy PR) also makes a case for using the floating-mean versus naive mean removal method. So, if you think it shouldn't be the default for scipy, I'll be curious to hear why.

@adammj

@adammj - we're just trying to understand why we get the failures with the recent code changes - no value judgement involved! But: are we correct in inferring that before the recent changes, there was no floating mean correction applied?

p.s. This wouldn't have been such a surprise if it hadn't been stated that the changes were backwards compatible. The changes would seem well worth a proper release note!

@adammj

@adammj - we're just trying to understand why we get the failures with the recent code changes - no value judgement involved! But: are we correct in inferring that before the recent changes, there was no floating mean correction applied?

Understood on the desire to understand what changed. And correct, before this change, the mean was assumed to be zero, and the user could use the precenter argument to naively remove the mean.

p.s. This wouldn't have been such a surprise if it hadn't been stated that the changes were backwards compatible. The changes would seem well worth a proper release note!

I guess I don't fall into the strictest camp of backwards compatibility, since it means no bugs can ever be fixed. Also, as far as I understand, the PR isn't in a scipy release yet, just the main branch. I'm also not sure what release note will be written for it.

I'm all for fixing things, and I understand that the change is backward compatible from a pure API perspective. And indeed the release is not yet there, so really all I meant to note is that people's science results will change with your PR. The change will be for the better, but I think should be mentioned in the release notes to warn people (even if not many people will see them...)

Back to the failing test, as per #17170 (comment) , we can close this issue when scipy nightly wheel catches up and we see green devdeps again. Thanks, all!

Thanks, all!

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	@pytest.mark.parametrize("method", ALL_METHODS_NO_AUTO)
	@pytest.mark.parametrize("center_data", [True, False])
	@pytest.mark.parametrize("fit_mean", [True, False])
	@pytest.mark.parametrize("errors", ["none", "partial", "full"])
	@pytest.mark.parametrize("with_units", [True, False])
	@pytest.mark.parametrize("normalization", NORMALIZATIONS)
	def test_all_methods(
	data, method, center_data, fit_mean, errors, with_units, normalization
	):

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

Description

Activity

neutrinoceros commented on Oct 9, 2024

neutrinoceros commented on Oct 9, 2024

adammj commented on Oct 9, 2024

pllim commented on Oct 9, 2024

mhvk commented on Oct 9, 2024

pllim commented on Oct 10, 2024

jakevdp commented on Oct 10, 2024

neutrinoceros commented on Oct 10, 2024

pllim commented on Oct 10, 2024

mhvk commented on Oct 10, 2024

jakevdp commented on Oct 10, 2024

mhvk commented on Oct 10, 2024

neutrinoceros commented on Oct 10, 2024

jakevdp commented on Oct 10, 2024

neutrinoceros commented on Oct 10, 2024

dhomeier commented on Oct 11, 2024

adammj commented on Oct 11, 2024

mhvk commented on Oct 11, 2024

adammj commented on Oct 11, 2024

mhvk commented on Oct 11, 2024

pllim commented on Oct 14, 2024

pllim commented on Oct 14, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions