pymc
pymc copied to clipboard
Implemented `rng_fn` to CAR/ICAR
Hi @jessegrabowski , @ricardoV94 !
I've implemented the rng_fn for the designated classes CAR/ICAR in the issue #7713 , so could you please review and let me know if there is anything needs to be changed .
Thank You !
📚 Documentation preview 📚: https://pymc--7723.org.readthedocs.build/en/7723/
]
:sparkling_heart: Thanks for opening this pull request! :sparkling_heart: The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.
Unless there's some subtle complexity I would suggest to implement as a SymbolicRV like in: https://github.com/pymc-devs/pymc/pull/7685
Instead of doing all the decomposition here, just pass the precision matrix and set
method = "eig"tomultivariate_normal.You will also need to add a test.
Hi @jessegrabowski !
Yeah sure I'll do that . Is there any specified file for the test or I've to create a new file ?
Thank You !
Go into tests/distributions/test_multivariate.py::TestICAR::test_icar_rng_fn and update the test to no longer expect an error. Check the other test_rng_fn tests for how to validate the output.
It looks like you also have to run pre-commit
Go into
tests/distributions/test_multivariate.py::TestICAR::test_icar_rng_fnand update the test to no longer expect an error. Check the othertest_rng_fntests for how to validate the output.It looks like you also have to run pre-commit
Thanks @jessegrabowski for timely response!
I've updated the rng_fn and added the test as per your request. Let me know if anything needs to change ?
Once again Thank You !
Did you check the PR that Ricardo linked? This would be the preferred way to go.
Did you check the PR that Ricardo linked? This would be the preferred way to go.
You're referring that to write the rng_fn like SymbolicRV or something else?
You would convert the entire distribution into a SymbolicMVNormalUsedInternally subclass, like how MvStudentTRV was changed in the linked PR (check here specifically). Then no rng_fn is required at all. The rv_op signature will take all the inputs (mu, W, alpha, tau), and the extended_signature will be "[rng],[size],(n),(n,n),(),()->[rng],(n)", because mu is a vector, W is a matrix,andalphaandtau` are scalars, and it returns a vector.
Then do the same logic you already did in the rng_fn to compute draws, but symbolically in the rv_op. It will return:
return cls(
inputs=[rng, size, mu, W, alpha, tau],
outputs=[next_rng, draws],
method=method,
)(rng, size, nu, mean, scale)
The method argument should be eig by default.
You would convert the entire distribution into a
SymbolicMVNormalUsedInternallysubclass, like howMvStudentTRVwas changed in the linked PR (check here specifically). Then no rng_fn is required at all. Therv_opsignature will take all the inputs (mu, W, alpha, tau), and theextended_signaturewill be"[rng],[size],(n),(n,n),(),()->[rng],(n)", becausemuis a vector,Wis a matrix,andalphaandtau` are scalars, and it returns a vector.Then do the same logic you already did in the
rng_fnto computedraws, but symbolically in therv_op. It will return:return cls( inputs=[rng, size, mu, W, alpha, tau], outputs=[next_rng, draws], method=method, )(rng, size, nu, mean, scale)The
methodargument should beeigby default.
Hey @jessegrabowski ! Hope you are fine !
I've implemented that part but struggling with test part . Can you help me out with and tell me that what I'm missing ? Here's the link of the code file & error
Thank You !
You can push the code you have so far and we can look at it here
Hi @Muhammad-Rebaal , are you still working on this ? If not, do you mind if I take a look at it ? Thank you 🙏