Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace MatrixDirichlet with TensorDirichlet #227

Open
wouterwln opened this issue Jan 22, 2025 · 4 comments
Open

Replace MatrixDirichlet with TensorDirichlet #227

wouterwln opened this issue Jan 22, 2025 · 4 comments

Comments

@wouterwln
Copy link
Member

TensorDirichlet is a strict generalization of MatrixDirichlet. Since we optimized it a bit, it is also a bit faster. e.g.:

α = rand(100,100)
z = rand(100, 100)
d1 = TensorDirichlet(α)
d2 = MatrixDirichlet(α)

@benchmark logpdf($d1, $z)
@benchmark logpdf($d2, $z)
BenchmarkTools.Trial: 10000 samples with 7 evaluations per sample.
 Range (min … max):  4.375 μs … 472.524 μs  ┊ GC (min … max): 0.00% … 98.29%
 Time  (median):     4.423 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   4.547 μs ±   4.687 μs  ┊ GC (mean ± σ):  1.02% ±  0.98%

  ▄█▇▆▄▃▂▂▁  ▂▃▄▃▅▅▅▅▃▄▃▃▃▁▂▁▁▁ ▁                             ▂
  █████████▇██████████████████████▇▇▆▆▆▇▇▅▇▆▇▇▆▆▇▇▇▆▆▆▇▅▅▆▆▅▄ █
  4.38 μs      Histogram: log(frequency) by time      5.01 μs <

 Memory estimate: 944 bytes, allocs estimate: 3.

BenchmarkTools.Trial: 10000 samples with 1 evaluation per sample.
 Range (min … max):  175.250 μs …   2.661 ms  ┊ GC (min … max): 0.00% … 91.93%
 Time  (median):     179.792 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   187.954 μs ± 115.324 μs  ┊ GC (mean ± σ):  3.10% ±  4.67%

       ▃▇█▅▁                                                     
  ▁▁▂▃▆█████▆▅▄▄▃▃▃▂▂▃▅▆▆▅▅▄▄▄▄▃▃▂▃▂▂▂▂▂▂▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▂
  175 μs           Histogram: frequency by time          201 μs <

 Memory estimate: 186.12 KiB, allocs estimate: 403.

I think we should replace everything before we get caught up in MatrixDirichlet even further. This also simplifies the implementation of Transition in RMP. What do you think @bvdmitri @Nimrais ?

@bvdmitri
Copy link
Member

I think it makes sense, also helps to avoid confusion with an actual MatrixDirichlet distribution

@ismailsenoz
Copy link
Collaborator

There is still a confusion with the naming conventions. Neither MatrixDirichlet nor TensorDirichlet are good names for the underlying distributions. The distributions we currently have are variety of independent multiplications of Dirichlet distribution. Matrix Dirichlet is independent Dirichlet over columns. The naming stems from ForneyLab that should be changed. I am in favor of only retaining what is called TensorDirichlet. Nevertheless, we need to be aware this is not a good name either as it only models indpendent Dirichlets organized as a tensor. We should discuss a good naming convention.

I thought the implementation was going to generalize the Dirichlet distribution to matrices and tensors. Nevertheless, it doesn't do that. We need to implement the actual generalizations to call them TensorDirichlet or MatrixDirichlet.

@wouterwln
Copy link
Member Author

@ismailsenoz I agree, only I haven't come up with a better alternative. Maybe something like DirichletCollection?

@ismailsenoz
Copy link
Collaborator

I like the suggestion. Then we would need a type that can handle MatrixVariate and TensorVariate simultaneously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants