Fix `pm.distributions.transforms.ordered` fails on >1 dimension

[purna135]

Addressing #5659

Kept the original dims in the log_jac_det by changing

def log_jac_det(self, value, *inputs): 
     return at.sum(value[..., 1:], axis=-1)

to

def log_jac_det(self, value, *inputs):
    return at.sum(value[..., 1:], axis=-1, keepdims=True)

[purna135]

Hello @ricardoV94, after making these changes I ran the code mentioned in the issue, and it ran without error this time. However, I am unsure how to select the appropriate test for this; could you please assist me with this?

[ricardoV94]

You can just add the original issue example as a new test to test_transforms.py and assert the result of point_logps is what's expected.

[codecov[bot]]

Codecov Report

Merging #5660 (c42288c) into main (fa015e3) will increase coverage by 0.34%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5660      +/-   ##
==========================================
+ Coverage   86.86%   87.21%   +0.34%     
==========================================
  Files          75       75              
  Lines       13715    13715              
==========================================
+ Hits        11914    11961      +47     
+ Misses       1801     1754      -47     

Impacted Files	Coverage Δ
pymc/distributions/transforms.py	59.63% <0.00%> (-40.37%)	:arrow_down:
pymc/distributions/dist_math.py	57.71% <0.00%> (-29.72%)	:arrow_down:
pymc/distributions/logprob.py	69.72% <0.00%> (-25.69%)	:arrow_down:
pymc/data.py	63.02% <0.00%> (-20.59%)	:arrow_down:
pymc/aesaraf.py	87.43% <0.00%> (-1.01%)	:arrow_down:
pymc/parallel_sampling.py	86.04% <0.00%> (-0.67%)	:arrow_down:
pymc/sampling.py	88.15% <0.00%> (+0.11%)	:arrow_up:
pymc/distributions/multivariate.py	92.15% <0.00%> (+0.12%)	:arrow_up:
pymc/model.py	85.89% <0.00%> (+0.27%)	:arrow_up:
... and 10 more

[purna135]

test has been added, please have a look.

[drbenvincent]

Thanks for the contribution so far @purna135 ! I have my eye on this, and it looks like there are just two failing tests before this can be merged?

[purna135]

Thank you so much, @drbenvincent; I was hoping for a response. Yes, some tests are failing, but I can't figure out how to fix them; could you please assist me?

[drbenvincent]

Thank you so much, @drbenvincent; I was hoping for a response. Yes, some tests are failing, but I can't figure out how to fix them; could you please assist me?

I would gladly, if I could, but so far my contributions have been to pymc-examples, and not to pymc itself. So I do not have enough experience. Maybe @ricardoV94 could give some hints when he has time?

[ricardoV94]

@purna135 Have you tried to investigate why those tests are failing locally?

[purna135]

Yes, I ran the test locally, but I couldn't figure out what was causing it. I assumed that because we changed log_jac_det, the expectation of the following line changed as well. jacob_det = transform.log_jac_det(test_array_transf, *x.owner.inputs)

As a result, I think we should revise the assert statement.

[ricardoV94]

You can print the results before and after the changes to be sure what's changed. Then we have to figure out if we introduced a bug or if the old results were wrong (probably the former)

[purna135]

Is this jacob_det the one I should print to check the results?

[purna135]

Hi @ricardoV94 I'm still waiting for a response and am unsure how to proceed. Could you please assist me?

[drbenvincent]

Hi. Just dropping a message here to see if anyone is able to push this forward? I have a vested interest in this fix, but am not familiar enough to help contribute efficiently.

[TimOliverMaier]

Hey! @purna135, is there any news on this? I would also be interested in this fix. I tried to add the suggestions into the code, but I am still having failing tests and tbh a lack of understanding, as well. I would be happy to help here, but I can't accomplish it by myself, too.

[TimOliverMaier]

I've been advancing on this locally. There are two remaining issues, as far as I see:

1. There are some remaining failing tests that are caused by the changing dimensionality of the jacobian determinant <- Some Transforms keep the last dimension, others do not. Keeping the other transforms as they are will make some further cases in test_transform.check_vectortransform_elementwise_logp() necessary.
2. Sampling of ordered multivariate distributions fails with ValueError : "array must not contain infs or NaNs\nApply node that caused the error: SolveTriangular..." However, this is also the case on the main branch for me. What is the best way to collaborate on this PR? @purna135

[purna135]

Hello, @TimOliverMaier. Thank you so much for addressing this issue and sorry for the delay in responding; I was preoccupied with other issues and didn't have time to solve this one. It would be greatly appreciated if you could open a new PR from your local branch and I would close this one.

[TimOliverMaier]

Hey, I opened a new PR. #6255 It is still work in progress, though.

[ricardoV94]

Fixed in #6255