mshawcroft commented on a change in pull request #9682:
URL: https://github.com/apache/tvm/pull/9682#discussion_r765072489
##########
File path: python/tvm/relay/op/contrib/cmsisnn.py
##########
@@ -227,7 +227,7 @@ def binary_op_pattern(op):
def check_qnn_binary_op(extract):
"""Check if multiply is supported by CMSIS-NN."""
- return (
+ return bool(
Review comment:
Ah ok, that's curious, and presumably once one of those subclauses has
an integer type, the subsequent application of and just propagates the
numerical type rather than giving a bool.
Thanks for the explanation.
This explanation makes sense for the conv2d case where that padding
comparison return intimm, but presumably it does not apply to several of the
other cases where bool() has been inserted. For instance this qnn_binary_op
case that I commented on... in which case it would probably be some what less
confusing to the future reader of this code if we didn't insert the unnecessary
bool() cast.
For the case(s) where == return intimm instead of bool, I wonder whether
this is the actual bug, or whether the actual bug is that the underlying ==
machinery is broken and returning an inappropriate type, it is certainly not
illegal to have == return an arbitrary object in python, but there is a strong
convention that __eq__ returns a bool, Im thinking specifically from the
perspective of [the principle of least astonishment
](https://en.wikipedia.org/wiki/Principle_of_least_astonishment).
If this really is the bug (and == really should be returning a non bool
object), then I'd suggest that from the readers perspective, inserting bool
cast just around the subexpression that need to be cast rather than around
entire subexpression is less confusing. A comment as to why the spurious
bool() cast is required might also be helpful.....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
