-
Notifications
You must be signed in to change notification settings - Fork 409
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
No longer make ExponentPrecisionType and XnorPrecisionType inherit from IntegerPrecisionType #845
Conversation
About the pytest failure, Quartus Winograd is broken for Xnor inputs. #804 forces im2col instead of Winograd for this reason. I can hide this failure if desired. |
""" | ||
Convenience class to differentiate 'regular' integers from BNN Xnor ones | ||
""" | ||
|
||
def __init__(self): | ||
super().__init__(width=1, signed=False) | ||
|
||
def __str__(self): | ||
typestring = 'uint<1>' |
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 am truthfully not much of a fan of the typestring. What is it's function? Is it to have a string representation in the configs? There is not a 1:1 between a typestring and and the type. Part of me thinks this should be 'xnor' and not 'uint<1>', though this requires a few more changes downstream. (There would be a corresponding change for exponent precision types.)
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.
Not 100% sure, but this may be the last leftover from the previous way of doing things. I think that it is only used in WeightVariable.update_precision()
to set a new string format that is then not used anywhere (it was used only in print_array_to_cpp
in the writer, but this was changed). If it doesn't hurt your eyes too much I would leave it as-is here, and have a follow up PR that removes it, size_cpp()
and perhaps the whole dim_names
concept as one of our usual "cleanup" PRs that we do prior to a release.
@@ -311,16 +314,20 @@ def __eq__(self, other): | |||
return eq | |||
|
|||
|
|||
class XnorPrecisionType(IntegerPrecisionType): | |||
class XnorPrecisionType(PrecisionType): | |||
""" | |||
Convenience class to differentiate 'regular' integers from BNN Xnor ones | |||
""" | |||
|
|||
def __init__(self): | |||
super().__init__(width=1, signed=False) |
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.
Logically xnor is signed, even if we represent it as unit<1>. How it gets implemented should be a backend issue, not a definition issue. Leaving it unsigned for now, though, since that's a bigger change.
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 the XNOR idea was that it is a single bit implementation, the more general definition of BNN is signed (-1 and 1), but that's a different case.
I think after #804 is merged the pytest failure will go away. But I am curious what people think about the "review" comments I made. |
Description
When trying to propagate precisions,
isinstance(var, (IntegerPrecisionType, FixedPrecisionType))
produces incorrect results if the variable isExponentPrecisionType
orXnorPrecisionType
. The "is a" requirement for inheritance is violated. Therefore it's better to remove the inheritance, which this PR does. It also cleans up a bit of the remaining string parsing there was and switches from the old%
string formatting to f-strings or.format()
.Type of change
It is not really a "bug" but the previous inheritance structure caused issues with yet to be added precision propagation.
Tests
This should not break anything. The qkeras tests in particular are the main tests for this.
Checklist
pre-commit
on the files I edited or added.