-
Notifications
You must be signed in to change notification settings - Fork 3
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
[Observers] group size + channel wise + per token #32
Conversation
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 main thing I see missing here is that we aren't actually using the strategy field of QuantizationArgs
. It makes sense to support group_size=-1
as channelwise but I think the code would be more readable if instead of checking for group size we could just check for the QuantizationArgs.strategy enum. This would make it easier to extend when we add the token
strategy too.
Maybe we could add a validator to QuantizationArgs so if the user specifies group_size=-1 we automatically set channel as the strategy
Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com>
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.
let's also add at least a simple test for each strategy that validates a forward pass runs and scales/zero points have the expected shape
Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com>
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.
LGTM once the test failures are fixed!
starter script