Improve string representation of Assert Ops #739
Conversation
ricardoV94
force-pushed
the
improve_assert_str_repr
branch
from
f56cd76 to
973c1d5
Compare
| @@ -48,7 +48,13 @@ def __init__(self, exc_type, msg=""): | |||
| self.msg = msg | |||
|
|
|||
| def __str__(self): | |||
| return f"CheckAndRaise{{{self.exc_type}({self.msg})}}" | |||
| name = self.__class__.__name__ | |||
There was a problem hiding this comment.
Will the name variable not always be instance of CheckAndRaise in itself here? If so what is the benefit of having a separate variable for it?
There was a problem hiding this comment.
If there is a subclass it will use the subclass name instead
| if len(self.msg) > 30: | ||
| msg = self.msg[:27] + "..." | ||
| else: | ||
| msg = self.msg |
There was a problem hiding this comment.
I am a bit conflicted here. Why do we want to contain the error message? I understand that it makes the code more readable but it would also hide some important information regarding the message. Although the length of 27 again seems sufficient when displaying the error message.
There was a problem hiding this comment.
This is just for the text representation (like when doing pytensor.dprint). If the assert fails during execution, the whole message will be print out.
Right now showing the whole message makes graph printing pretty ugly. That's the whole point of this PR
|
Need to fix some tests that depended on the graph representation of |
Hi @ricardoV94 . I have solved the error with these changes locally. However, I am unable to push the changes to your fork of the branch in order for the changes to appear here. Should I open a separate PR and reference it here? |
|
Contd in #891 |
|
This should be closed if it is superseded by #891 |
Description
Makes graphs with Assert a bit more readable
Checklist
Type of change