Closed nabriis closed 1 month ago
@amal-ghamdi I have now implemented the conjugacy safety framework which is very strongly based on what Jasper did. Could you do a technical review?
Also @jeverink, it would be great if you want to have a look. In particular I was wondering if you have a case that we use this _regression_test_linear
method? Else I will remove it for now. I am not even sure the conjugate sampler supports a linear operator on the function.
Thanks for the work @nabriis , here is my take on _regression_test_linear
.:
In terms of conditioning (I believe) we should be able to handle I can quickly come up with:
The current version of this PR can only handle cases 1 and 4, by checking using _check_conjugate_paramter_is_scalar_identity
for the prec
or using _check_conjugate_parameter_is_scalar_reciprocal
for cov
.
If I recall correctly, replacing the identity check with a linearity check, i.e., _check_conjugate_paramter_is_scalar_identity
with some improved version of _regression_test_linear
, could allow for case 2 . This additional scaling of the parameters is something I do plan to use as part of #424.
I suspect that a final generalization to case 5 and cases 3 and 6 (if wanted) should not be too much additional work then.
(Please fix the paramter
typo in the code)
Thanks for the work @nabriis , here is my take on
_regression_test_linear
.:In terms of conditioning (I believe) we should be able to handle I can quickly come up with:
1. prec = lambda d : d 2. prec = lambda d : 10*d 3. prec = lambda d : 10_d_np.eye(n) (or any other matrix) or 4. cov = lambda d : 1.0/d 5. cov = lambda d : 0.1/d 6. cov = lambda d : (0.1/d)*np.eye(n) (or any other matrix)
The current version of this PR can only handle cases 1 and 4, by checking using
_check_conjugate_paramter_is_scalar_identity
for theprec
or using_check_conjugate_parameter_is_scalar_reciprocal
forcov
.If I recall correctly, replacing the identity check with a linearity check, i.e.,
_check_conjugate_paramter_is_scalar_identity
with some improved version of_regression_test_linear
, could allow for case 2 . This additional scaling of the parameters is something I do plan to use as part of #424.I suspect that a final generalization to case 5 and cases 3 and 6 (if wanted) should not be too much additional work then.
(Please fix the
paramter
typo in the code)
@nabriis, I currently think that _regression_test_linear
can be removed and that handling the cases 1 and 4 above can be enough for the current PR, as it does not break any current models (as far as I am aware) and the other extension can than be considered in future PR.
Thanks for the work @nabriis , here is my take on
_regression_test_linear
.: In terms of conditioning (I believe) we should be able to handle I can quickly come up with:1. prec = lambda d : d 2. prec = lambda d : 10*d 3. prec = lambda d : 10_d_np.eye(n) (or any other matrix) or 4. cov = lambda d : 1.0/d 5. cov = lambda d : 0.1/d 6. cov = lambda d : (0.1/d)*np.eye(n) (or any other matrix)
The current version of this PR can only handle cases 1 and 4, by checking using
_check_conjugate_paramter_is_scalar_identity
for theprec
or using_check_conjugate_parameter_is_scalar_reciprocal
forcov
. If I recall correctly, replacing the identity check with a linearity check, i.e.,_check_conjugate_paramter_is_scalar_identity
with some improved version of_regression_test_linear
, could allow for case 2 . This additional scaling of the parameters is something I do plan to use as part of #424. I suspect that a final generalization to case 5 and cases 3 and 6 (if wanted) should not be too much additional work then. (Please fix theparamter
typo in the code)@nabriis, I currently think that
_regression_test_linear
can be removed and that handling the cases 1 and 4 above can be enough for the current PR, as it does not break any current models (as far as I am aware) and the other extension can than be considered in future PR.
Thanks for the input. I agree. I added #453
Hi @amal-ghamdi and @jakobsj. As discussed this PR is now ready for you both :)
Thank you very much @nabriis and @jeverink for joint work for adding safety checks on the conjugate sampler. The solution with adding conjugate pair as a base class and specific concrete pair classes is very neat. I see also comprehensive tests are added, and a demonstration of the check in the demo, very nice.
My only request here, is for some general documentation to be added, as especially now with the introduction of the pair class, there are many concepts in play. Specifically, I think it should be explained somewhere:
- what conjugacy is,
- define what a conjugate pair is,
- in ConjugatePair.sample() is is mentioned "sample from the conjugate distribution" which in singular is unclear when we need a pair of conjugate distributions, what is meant by "THE conjugate distribution"
- "conjugate parameter"
I don't know where's the best place to put this, docstring of the ConjugatePair ABC perhaps since general, though may not be so visible there. Maybe in docstring of ConjugateNew sampler?
There is a comment in the docstring of ConjugateNew that the sampler does NOT check correctness, can this comment be removed now this PR adds check for correctness?
I updated the description now. Hope it helps. I think we could write a good tutorial later on conjugacy, but perhaps this is sufficient for this PR.
Closes #345 Closes #174
Best reviewed in "side-by-side" mode.
ToDo