Fix extension() and localize() giving wrong results with fermitools 2.4-2.5.2 - #680
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #680 +/- ##
==========================================
- Coverage 51.21% 50.89% -0.32%
==========================================
Files 145 145
Lines 27443 27469 +26
==========================================
- Hits 14054 13980 -74
- Misses 13389 13489 +100
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gta.extension()andgta.localize()silently returned incorrect results with Fermi ScienceTools 2.4.0 through 2.5.2. Extension fits reported nonsensical best-fit values withnanerrors and ~zeroTS_ext; localization reported positional uncertainties several times too large. Both worked correctly with fermitools 2.2.0.This was caused by a regression in fermitools itself, now confirmed fixed in fermitools 2.5.3 (currently available as a
fermi/label/devconda build). This PR:Symptom
Running the IC 443 extension tutorial:
Expected (matches Abdo et al. 2010, and what fermipy 1.4.1 / fermitools 2.2.0 produces):
Same pattern in
localize(): for a TS~42 point source, reportedpos_err/r68were ~5-10x too large (0.38°/0.58° vs. the expected ~0.04-0.1°), with the position refinement scan'sDeltaLogLikepinned near 0.Root cause (upstream, in fermitools)
Both
extension()andlocalize()implement fast width/position scans by callingBinnedLikelihood.setSourceMapImage()to overwrite a source's per-pixel counts map in place at each trial value, then re-fitting the normalization — this avoids the overhead of deleting and re-adding the source at every step.A fermitools regression, introduced in
fermi-lat/Likelihood@9f574d768("Fix slowdown by reverting reloadIfCleared()", 2024-08-22, first shipped in fermitools 2.4.0), causedSourceMap::reloadIfCleared()to no longer reset its internalm_dataClearedflag after regenerating a source's map. SinceBinnedLikelihood::addSourceCounts()callsreloadIfCleared()unconditionally on every source on every likelihood evaluation, andm_dataClearedgets set the first time any source transitions out of the fixed-source list (a routine event — e.g. every sourceGTAnalysis.optimize()touches), the flag effectively became stucktrueforever afterward. From that point on, the affected source's map was silently regenerated from its original spatial definition on every fit, discarding anything written viasetSourceMapImage().We verified this directly: overwriting a free source's map and reading it back before vs. after a
fit()call showed the map correctly reflecting the injected value pre-fit, then reverting to a fixed value post-fit regardless of what was injected — withSourceMap.model_is_localflippingTrue->Falseacross the fit.Filed upstream:
<link to fermitools issue, if filed>. Confirmed fixed in fermitools 2.5.3.What changed
Since the fast (
use_pylike=False) path is unsafe on fermitools < 2.5.3, but correct (and considerably faster) on 2.5.3+, this PR adds a runtime version check and only forces the slow (use_pylike=True, recreate the source) path when needed.fermipy/gtutils.py(new)FERMITOOLS_SRCMAP_FIX_VERSION = (2, 5, 3).fermitools_srcmap_update_is_broken(): parsesfermipy.get_st_version()and returns whether the installed ScienceTools predates the fix. Cached (lru_cache) since the installed version doesn't change at runtime. Fails safe (assumes the bug is present) if the version string can't be parsed.use_pylike_srcmap_workaround(logger=None): the function callers actually use. Returns the same boolean, and — the first time (and only the first time) it returnsTruein a process — logs alogger.warning(...)explaining that the slower method is being used and that upgrading to fermitools >= 2.5.3 restores the fast path.fermipy/extension.py_scan_extension_fast/_scan_extension_fast_ebin:use_pylike = reoptimize and use_pylike_srcmap_workaround(self.logger). The fast in-place path is used whenever just evaluating the likelihood without a re-fit (reoptimize=False, always safe), or whenever a re-fit follows but the installed fermitools has the fix; forced to the recreate-source path only when a re-fit follows on a broken fermitools version.RuntimeError: Wrong size for input model mapon the old fast-path restore); otherwise restore the raw array directly as before.set_source_morphology(...)calls in_extension()/_fit_extension_full()that directly precede a_fit()call now useuse_pylike_srcmap_workaround(self.logger)instead of a hardcoded value.use_pylike=Falseoverride on the_fit_position(...)call in_fit_extension_full(thefit_position=Truecombined mode), which was also unnecessarily forcing the TS-map coarse-localization step into the unsafe path.fermipy/sourcefind.py_scan_position(used bylocalize()'s refinement scan and by_fit_position()):use_pylike = use_pylike_srcmap_workaround(self.logger)(this loop always re-fits, so noreoptimizegate is needed here). Removed the now-dead_create_srcmap_cachecall that only served the fast path when it was unconditionally disabled.Validation
IC 443 extension fit (
3FGL J0617.2+2234e), width scan[0.25, ..., 0.30]:On fermitools 2.5.3 the result is effectively an exact match to the published value, and runtime is back to the original ballpark (~10s vs. ~120s under the forced workaround) — confirming both that the upstream fix works and that this PR correctly re-enables the fast path automatically.
localize()on3FGL J0619.4+2242(TS~42), fermitools 2.5.1 (workaround forced):No warning is logged on fermitools 2.5.3; the one-time warning was confirmed to fire correctly on 2.5.1/2.5.2.
Tested in three environments: python 3.11 / fermitools 2.5.1 (bug), python 3.12 / fermitools 2.5.2 (bug), python 3.12 / fermitools 2.5.3-dev (fixed).