TST: Add direct C-extension tests for BLS periodogram - #20151
Conversation
|
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.
|
|
|
||
| from astropy.timeseries.periodograms.bls._impl import bls_impl | ||
|
|
||
| _F64Array = npt.NDArray[np.float64] |
There was a problem hiding this comment.
this annotation will become unnecessary in the future but let's just be consistent with the existing ones for now.
| _F64Array = npt.NDArray[np.float64] | |
| _F64Array: TypeAlias = npt.NDArray[np.float64] |
| true_depth = 0.5 | ||
|
|
||
| t = np.linspace(0, 10, 1000, dtype=np.float64) | ||
| y = np.ones_like(t, dtype=np.float64) |
There was a problem hiding this comment.
The dtype will implicitly match t's
| y = np.ones_like(t, dtype=np.float64) | |
| y = np.ones_like(t) |
| 0, # obj_flag | ||
| ) | ||
|
|
||
| assert type(results) is tuple |
There was a problem hiding this comment.
In this particular case I think we'd rather make the type check a little looser to make room for a potential change using a NamedTuple
| assert type(results) is tuple | |
| assert isinstance(results, tuple) |
| for array_out in results: | ||
| assert type(array_out) is np.ndarray | ||
| assert array_out.dtype == np.float64 | ||
| assert array_out.shape == periods.shape |
There was a problem hiding this comment.
this seems good but probably insufficient. I would expect at least a couple numerical checks in addition to these.
| expected_err_msg: str, | ||
| ) -> None: | ||
| """Test C-level early-exit flags.""" | ||
| with pytest.raises(ValueError) as exc_info: |
There was a problem hiding this comment.
is there a reason not to use pytest.raises's match argument here ? The expected message also seems invariant so let's not overengineer it yet and avoid parametrizing on it
There was a problem hiding this comment.
Yeahh understandable, I should've done exactly that
| 0, | ||
| ) | ||
|
|
||
| out_objective, out_depth, _, out_duration, _, _, _ = results |
There was a problem hiding this comment.
I'd prefer to have unused return values be named (use _ as a prefix instead of the whole name) to make the test more readable in isolation. It'd also be good to have a comment or two explaining why they are not checked.
| best_idx = np.argmax(out_objective) | ||
|
|
||
| np.testing.assert_allclose( | ||
| periods[best_idx], perfect_transit.true_period, rtol=1e-2 |
There was a problem hiding this comment.
these relative tolerances seem a bit underwhelming. Is there a technical reason why we expect low accuracy here ?
There was a problem hiding this comment.
The 1e-2 tolerance was originally a sort of bandage for two things: first, my synthetic data only had 1,000 points with an oversample=10 rate, and second my np.linspace(1.5, 2.5, 100) didn't actually include the exact period of 2.0..... hence why I decided to go for rtol=1e-2 but I fixed that in the latest commit.
3 participants
Hey everyone! This PR tackles the BLS extension testing for my GSoC project.
Instead of routing through the high-level
BoxLeastSquaresAPI, this suite hits the Cython/C-engine (bls_implandrun_bls) directly to ensure the math and memory allocations hold up in complete isolation.Key additions:
np.float64memory buffers directly into the C-boundary.SyntheticTransitfixture to mathematically verify the C-engine recovers the correct period, depth, and duration from a planted signal. (Note: I relaxed thertolfor the depth assertion to5e-2to account for expected discrete phase binning drift across the C-level grid).ValueErrorsas expected.cc: @neutrinoceros