-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
chore: adding NoneConst and None check #7764
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
base: main
Are you sure you want to change the base?
Conversation
|
@@ -289,7 +289,13 @@ def find_measurable_index_mixture(fgraph, node): | |||
# We don't support (non-scalar) integer array indexing as it can pick repeated values, | |||
# but the Mixture logprob assumes all mixture values are independent | |||
if any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole check is a bit of a mess we should think a bit more. It could also be a non-constant slice (see if not
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardoV94 by this line it could also be a non-constant slice
do you mean to say that dynamic slices (non-constant slice
) are also need to be skipped?
Also what is your view if we check for tensor variable and only in that case graph node get modified? (solution for mess)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check should be just something like
if any(
(
isinstance(indices, TensorVariable)
and indices.dtype.startswith("int")
and any(not b for b in indices.type.broadcastable)
)
)
for indices in mixing_indices
):
Basically rulling out potentially repeated integer indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardoV94 are you saying to modify graph node in above condition only,
if it is true (which i understand) then we have to use all not any:
if all( // all not any
(
isinstance(indices, TensorVariable)
and indices.dtype.startswith("int")
and any(not b for b in indices.type.broadcastable)
)
)
for indices in mixing_indices
):
// modify the nodes
else :
None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that if any
I suggested is the one leading to return None
. No need for a new branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then in None and NoneConst cases it is going to modify node which is not recommended in issue description :
I am running below code
from pytensor.tensor.type_other import NoneConst, NoneTypeT, SliceConstant, SliceType
from pytensor.tensor import TensorVariable
mixing_indices = [None, NoneConst]
if any(
(
isinstance(indices, TensorVariable)
and indices.dtype.startswith("int")
and any(not b for b in indices.type.broadcastable)
)
for indices in mixing_indices
):
print("return none")
else:
print("modify node")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing we should have to worry about is repeated indices, None
/NoneConst
don't introduce repeated indices, just a dummy dimension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in issue:
The [line that I quoted above](https://github.com/pymc-devs/pymc/blob/main/pymc/logprob/mixture.py#L294) needs to also check if the indices are None or [NoneConst](https://github.com/pymc-devs/pytensor/blob/main/pytensor/tensor/type_other.py#L132). That way, the rewrite will return None when it has a mixture of integer indexes, and slices or new axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ricardoV94 i have done the changes, I am new to codebase (hope you understand), that is why having that much of questions.
Thanks
Description
Fixes issue: #7762
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7764.org.readthedocs.build/en/7764/