Skip to content

Conversation

@Johnson9009
Copy link
Contributor

This is a very easy to understand bug, maybe the original commiter forgot to add the negative sign in the "else" branch, it will cause the "nbit" of "left_shift" operator be a negative number, e.g., "left_shift(%50, -1)".

@Johnson9009
Copy link
Contributor Author

Please @ZihengJiang, @vinx13 help to review, thanks.

@FrozenGene
Copy link
Member

How about adding one unit testing?

@FrozenGene FrozenGene added the status: need test case need test cases to cover the change label Feb 10, 2021
@tqchen tqchen merged commit 25bf449 into apache:main Mar 3, 2021
@tqchen
Copy link
Member

tqchen commented Mar 3, 2021

Merging this in as the fix is quite obvious. @Johnson9009 It would be great if you can send a followup test case that covers the regression.

@Johnson9009 Johnson9009 deleted the quantize_negative_left_shift branch March 4, 2021 13:08
@Johnson9009
Copy link
Contributor Author

@tqchen @FrozenGene Sorry for late reply because of lots of work, yes it is great to add a test case in regression, I will try to add one as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need test case need test cases to cover the change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants