Always save g_idx when initialized in quantization compressor #467
Conversation
Remove conditional skip for g_idx saving in compression. Since g_idx is only initialized when needed, we should always save it when present rather than checking for uninitialized values. Signed-off-by: Rahul Tuli <[email protected]>
rahul-tuli
requested review from
Copilot,
brian-dellabetta,
shanjiaz and
nm-automation
and removed request for
brian-dellabetta
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the quantization compression logic by removing a conditional check that prevented saving g_idx parameters when they contained uninitialized values. Since g_idx is only initialized when needed for specific quantization schemes, the existing initialization logic already handles when it should be created, making the conditional skip unnecessary.
- Removes 8-line conditional block that skipped
g_idxsaving when values were <= -1 - Removes associated TODO comment questioning whether the uninitialized case occurs
- Ensures all necessary quantization parameters are preserved during compression
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
This makes sense to me based on your rationale, given the value never gets initialized earlier in the lifecycle. I have a similar concern with my #432 PR, how confident are we that the tests cover all these life cycle edge cases?
Summary
g_idxsaving inBaseQuantizationCompressorg_idxwhen present since it's only initialized when neededRationale
The
g_idxparameter is only initialized when it's needed for the quantization scheme. Since the initialization logic already handles when it should be created, there's no need to conditionally skip saving it based on uninitialized values. This simplifies the compression logic and ensures all necessary quantization parameters are preserved.Changes
g_idxsaving when values were <= -1Signed-off-by: Rahul Tuli [email protected]