Repository navigation
Conversation
A freshly constructed Model allocated its _parameters array with np.empty() and only populated it on the first access of the parameters property. Until then, _parameters held uninitialized memory. When that memory happened to contain a NaN, strict-equality operations that read the raw array directly -- such as check_pickling_recovery() comparing a pristine model against its unpickled copy -- failed non-deterministically. This was the root cause of the flaky Gaussian1DKernel pickle test disabled in astropy#18596. Populate _parameters from the actual parameter values immediately after allocation, re-enable the Gaussian1DKernel pickle test, and add regression tests covering both the initialization invariant and a strict pickle round-trip. Fixes astropy#18598
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.
|
This branch has not been deployed
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.
What this fixes
Fixes #18598.
A freshly constructed
Modelallocated its_parametersarray withnp.empty()in
_initialize_slices()and only populated it lazily, on the first access of theparametersproperty. Until that first access,_parametersheld uninitializedmemory.
That is harmless for the normal fitting workflow (which always touches
.parameters), but it makes any operation that reads the raw_parametersarraydirectly non-deterministic. The clearest victim is pickle-recovery testing:
astropy.tests.helper.check_pickling_recovery()compares the original andunpickled objects attribute-by-attribute with strict equality. When the
uninitialized memory happened to contain a NaN, the comparison
original._parameters == unpickled._parametersproducedFalse(sinceNaN != NaN), raisingAssertionError: Value of _parameters changed by pickling.This is exactly the intermittent failure that led to
Gaussian1DKernelbeingdisabled from
convolution/tests/test_pickle.pyin #18596 —Gaussian1DKernelwraps a
Gaussian1D, so the modeling-level flakiness surfaced there. As #18598notes, fixing
Gaussian1Dfixes the kernel automatically.The fix
Populate
_parametersfrom the actual parameter values immediately after it isallocated, so a pristine model never holds uninitialized memory:
_parameters_to_array()only needs the_param_metricsslices (already builtearlier in
_initialize_slices) and the parameter values, so it is safe to callhere. After the change, a fresh
Gaussian1D()has_parameters == [1., 0., 1.](its real defaults) instead of garbage.
The disabled
Gaussian1DKernelcase inconvolution/tests/test_pickle.pyisre-enabled (this is the
Revert #18596item on the issue checklist).How it was tested
Reproduced the failure first (RED). With a fresh model whose uninitialized
_parameterswas forced to contain NaN,check_pickling_recovery()failed withthe reported signature:
Reverting the one-line fix in the working tree and running the new
test_fresh_model_parameters_are_initializedshows the raw array actually holdinguninitialized garbage rather than the parameter values:
With the fix applied (GREEN), the full suites pass:
pytest astropy/modeling/tests/→ 1690 passed, 1118 skipped (scipy-gated)pytest astropy/convolution/tests/→ 1560 passed, 36 skipped, 8 xfailedruff checkandruff format --check(pinnedv0.15.20) are clean on all threechanged files.
Two regression tests are added in
modeling/tests/test_pickle.py:test_fresh_model_parameters_are_initialized(asserts a pristine model's raw_parametersmatches its real values and contains no NaN) andtest_gaussian1d_strict_pickling_recovery(exercises the strictcheck_pickling_recoverypath across all pickle protocols).