Fix print name of LogitNormal and DiscreteWeibull#7935
Conversation
|
|
||
|
|
||
| class ChiSquared: | ||
| class ChiSquaredRV(RandomVariable): |
There was a problem hiding this comment.
We shouldn't add more Ops just for naming. Less Ops means we can sample in other backends / do rewrites with fewer building blocks
There was a problem hiding this comment.
In the last commit ok?
There was a problem hiding this comment.
Nope, that's just a temporary hack. As soon as you do something like model.copy(), or any other model transform it's going to be lost. Why do you need it to show up as ChiSquared in the graphviz?
It's just a parametrization. Similarly we don't write UnitNormal if it happens to have loc=0, scale=1, even if we offered a helper to create them?
There was a problem hiding this comment.
I was creating a pymc_to_preliz function with the inmediate goal of being able to call pz.plot() for both preliz and pymc distributions. I found this and assumed it was of broader interest to show proper names. But if not, I can try to solve it inside that function.
There was a problem hiding this comment.
I don't think naming should come at the cost of more code complexity/redundancy. This is just a helper that creates a specific Gamma and seems sensible to write it as such. Can't you call pz.plot on the gamma object that's returned?
PS: We can emphasize more in the docstrings that this is just a helper that return a regular Gamma distribution
There was a problem hiding this comment.
Yes, the call to pz.plot works fine. Because the ChiSquared is correctly converted into a Gamma PreliZ distribution, and then everything works. The only "issue" is that the result is a Gamma and not a ChiSquared. So this was more about avoiding potential confusion on the user side than fixing something not working.
There was a problem hiding this comment.
Do you want to add a comment to reduce confusion for future devs/users?
There was a problem hiding this comment.
Added. I will also add a note on the PreliZ side.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7935 +/- ##
==========================================
- Coverage 91.47% 89.33% -2.15%
==========================================
Files 116 116
Lines 18904 18904
==========================================
- Hits 17292 16887 -405
- Misses 1612 2017 +405
🚀 New features to boost your workflow:
|
ricardoV94
changed the title
LogitNormal and DiscreteWeibull
2 participants
dist.owner.op._print_name[0]was returning incorrect names for:ChiSquared was returning "Gamma", because it's implemented as a special case of Gamma.Not sure if there is a simpler way to fix the issue for ChiSquared.📚 Documentation preview 📚: https://pymc--7935.org.readthedocs.build/en/7935/