Test that PDSignalEnergyPDF splines integrate to 1 and NGC 1068 unblinding result#302
Test that PDSignalEnergyPDF splines integrate to 1 and NGC 1068 unblinding result#302tomaskontrimas wants to merge 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds regression tests around the public-data time-integrated point-source analysis to validate physics outputs (NGC 1068 unblinding) and energy-PDF normalization—supporting the integration-method changes discussed in #299.
Changes:
- Add a regression test that switches the analysis source to NGC 1068 and checks the unblinding fit results.
- Add a test that checks PDSignalEnergyPDF spline normalization by integrating over the energy range.
- Import
scipy.integratein the test module to support numerical integration in the new normalization test.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_change_source_and_unblind(self): | ||
| source = PointLikeSource(name='NGC 1068', ra=np.radians(40.67), dec=np.radians(-0.01), weight=1) | ||
| self.ana.change_source(source) | ||
| try: | ||
| rss = RandomStateService(seed=1) | ||
| with self.tl.task_timer('Call unblind for NGC 1068.'): | ||
| (TS, params_dict, status) = self.ana.unblind(minimizer_rss=rss, tl=self.tl) | ||
|
|
||
| logger.info(f'{self.tl}') | ||
| logger.info(f'TS = {TS:.7f}') | ||
| logger.info(f'ns_fit = {params_dict["ns"]:.7f}') | ||
| logger.info(f'gamma_fit = {params_dict["gamma"]:.7f}') | ||
| logger.info(f'minimizer status = {status}') | ||
|
|
||
| np.testing.assert_allclose(TS, 19.5117469, rtol=1e-5) | ||
| np.testing.assert_allclose(params_dict['ns'], 54.9606574, rtol=1e-5) | ||
| np.testing.assert_allclose(params_dict['gamma'], 3.0884301, rtol=1e-5) |
There was a problem hiding this comment.
This test adds a second full unblind() call on the same (expensive-to-construct) analysis. Given CI runs the full test suite across multiple Python versions, this can noticeably increase runtime. Consider consolidating into a single test (e.g., loop over the two sources with subTest) or otherwise marking this as a slow/integration check to avoid unnecessarily extending CI time.
There was a problem hiding this comment.
It is an integration test and I reuse the already constructed analysis instance
| )[0] | ||
| / pdf.f_e_spl.norm | ||
| ) | ||
| np.testing.assert_allclose(integral, 1.0) |
There was a problem hiding this comment.
np.testing.assert_allclose(integral, 1.0) uses the default (very tight) tolerances, which can make this normalization check flaky across platforms/BLAS/SciPy versions—especially if f_e_spl.norm is computed via a different integration scheme than quad. Use an explicit tolerance (rtol/atol) that reflects the expected numerical accuracy, and consider reducing the amount of quad work (e.g., check only a small representative subset of PDFs per dataset) to keep test runtime under control.
| np.testing.assert_allclose(integral, 1.0) | |
| np.testing.assert_allclose(integral, 1.0, rtol=1e-6, atol=1e-8) |
There was a problem hiding this comment.
By default it's rtol=1e-07, atol=0 and I want it to fail if that's not achieved
As discussed in #299