Assert ndim and number of dims match #7390
Conversation
ricardoV94
force-pushed
the
fix_ndim_ordered
branch
from
fde8233
to
fb11f01
Compare
| @@ -1263,7 +1256,7 @@ class OrderedLogistic: | |||
| def __new__(cls, name, eta, cutpoints, compute_p=True, **kwargs): | |||
| p = cls.compute_p(eta, cutpoints) | |||
| if compute_p: | |||
| p = pm.Deterministic(f"{name}_probs", p, dims=kwargs.get("dims")) | |||
| p = pm.Deterministic(f"{name}_probs", p) | |||
There was a problem hiding this comment.
This is not trivial. p may have more or less dims than the respective Categorical variable, and these may or not be broadcasting. I decided it was not worth the trouble for the time being. In general we still need to figure out an approach for automatic dims (see #6828)
There was a problem hiding this comment.
I think partial dim assignment is currently supported, so you could do something like dims=(None,) * dim_diff + ('categories', ), no? The last one at least is always known iiuc
There was a problem hiding this comment.
No this is not sufficient either because p could have a broadcastable dim on the same batch dimensions of the underlying categorical variable. pm.Categorical(..., p=[[0.5, 0.5]], dims="obs”). You cannot say p has dims ("obs", None) because it has shape (1, 2) not (2, 2)
There was a problem hiding this comment.
Or you're suggestion adding a "categories dim" regardless of the rest. That has another problem, what if the user creates 2 variables with different number of categories? Then again you have two dims that are incompatible (same name, different shape).
That can also be solved (by using the variable name) but I don't want to do it here, I just want to not break stuff for now.
Besides that was not what the code was doing, it seems like the biggest focus was on reusing the user provided dims, not auto naming the extra dim
There was a problem hiding this comment.
I was assuming that "categories" already was provided somewhere, not that we generate or auto-name anything.
But yes I understand this is a rabbit hole, focusing on not breaking things is fine.
There was a problem hiding this comment.
In that case the PR is ready for review :)
ricardoV94
force-pushed
the
fix_ndim_ordered
branch
2 times, most recently
from
05258f4
to
818e814
Compare
ricardoV94
force-pushed
the
fix_ndim_ordered
branch
from
818e814
to
81f25c1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7390 +/- ##
=======================================
Coverage 92.19% 92.19%
=======================================
Files 103 103
Lines 17214 17215 +1
=======================================
+ Hits 15870 15871 +1
Misses 1344 1344
|
Fixes bug where invalid number of dims was given to Deterministic introduced by OrderedLogistic/Probit
Description
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7390.org.readthedocs.build/en/7390/