pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Implemented `rng_fn` to CAR/ICAR

Open Muhammad-Rebaal opened this issue 8 months ago • 10 comments

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/

Muhammad-Rebaal avatar Mar 15 '25 17:03 Muhammad-Rebaal

Thank You Banner] :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.

welcome[bot] avatar Mar 15 '25 17:03 welcome[bot]

Unless there's some subtle complexity I would suggest to implement as a SymbolicRV like in: https://github.com/pymc-devs/pymc/pull/7685

ricardoV94 avatar Mar 16 '25 11:03 ricardoV94

Instead of doing all the decomposition here, just pass the precision matrix and set method = "eig" to multivariate_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 !

Muhammad-Rebaal avatar Mar 23 '25 11:03 Muhammad-Rebaal

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

jessegrabowski avatar Mar 23 '25 11:03 jessegrabowski

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

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 !

Muhammad-Rebaal avatar Mar 23 '25 14:03 Muhammad-Rebaal

Did you check the PR that Ricardo linked? This would be the preferred way to go.

jessegrabowski avatar Mar 23 '25 14:03 jessegrabowski

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?

Muhammad-Rebaal avatar Mar 23 '25 14:03 Muhammad-Rebaal

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.

jessegrabowski avatar Mar 23 '25 15:03 jessegrabowski

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.

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 !

Muhammad-Rebaal avatar Mar 25 '25 16:03 Muhammad-Rebaal

You can push the code you have so far and we can look at it here

jessegrabowski avatar Mar 26 '25 01:03 jessegrabowski

Hi @Muhammad-Rebaal , are you still working on this ? If not, do you mind if I take a look at it ? Thank you 🙏

asifzubair avatar Jul 25 '25 01:07 asifzubair