Replace np.product, removed in numpy 2, so ARD runs again - #37
Merged
Conversation
ARD.update_C_prior called np.product to get the number of coefficients. numpy removed that alias, and jax.numpy with it, so every ARD call raised AttributeError: ARD was not degraded, it was unusable. math.prod is the right replacement rather than np.prod: the result is a shape argument for sparsity_kernel and has to be a Python int, not a traced value. ARD had no tests, which is why this went unnoticed. Adds RF recovery tests for ARD and ARDFixedPoint, and ARD to tests/test_evidence.py, where it reproduces the exact Gaussian marginal to 4 decimal places.
Merged
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
1 participant
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.
ARD.update_C_priorasked for the number of coefficients withnp.product(self.dims). numpy removed that alias in 2.0 andjax.numpywentwith it, so every call raised
ARDwas not producing worse answers, it was producing none. It is exportedfrom top level and is the first line of the
EvidenceOptdocs in the README.math.prodrather thannp.prod: the value is thencoeffargument tosparsity_kernel, which uses it as a shape, so it has to be a Python int andnot something that could arrive traced.
Why nobody noticed
ARDhad no tests.test_asd.pyandtest_ald.pyexist,test_ard.pydidnot, so the only thing standing between a removed numpy alias and a broken
estimator was somebody happening to run it.
tests/test_ard.py, new: RF recovery forARDand forARDFixedPoint. Thetwo
ARDtests fail on master with theAttributeErrorabove.tests/test_evidence.pygainsARD, againstlog N(y; 0, sigma^2 I + X C X'). It matches to 4 decimal places(
1112.2579both ways). This is the first time that objective could beevaluated at all, and it confirms that Fix evidence: use the posterior precision in the quadratic term #32 and Scale the prior covariance jitter to the prior, and return C with its own inverse #33 carry over to the ARD
path, which until now was assumed rather than checked.
ARDFixedPointis a separate implementation that never touchednp.productand was never broken. Its test is there because the class had no coverage
either, not because this changes anything for it.
Test status
69 passed, nothing failing.