-
Notifications
You must be signed in to change notification settings - Fork 233
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
Tensor for PTQ #2058
Tensor for PTQ #2058
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2058 +/- ##
===========================================
+ Coverage 36.13% 36.33% +0.19%
===========================================
Files 480 480
Lines 42998 43223 +225
===========================================
+ Hits 15539 15703 +164
- Misses 27459 27520 +61
|
7d1429d
to
9cc7582
Compare
def squeeze(a: TTensor, axis: Optional[Union[int, Tuple[int]]] = None) -> TTensor: | ||
def squeeze(a: TTensor, axis: Optional[Union[int, Tuple[int]]] = None) -> Tensor: |
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.
Could you please explain why do you change this?
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.
Functions in this file always return Tensor or list of Tensor
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 this is not entirely correct. I mean these functions return a Tensor
object only if there is no registered version for the type of the passed argument.
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.
Update all return type to Tensor
to correctly works "pop-up suggestions" (or how it names when editor suggest functions by first symbols?).
So set Tensor type to first argument. But not sure about second, added like Union[torch.Tensor, float], because it can be used like torch.tensor(1) + 1
or fns.min(torch.tensor(1), 0)
. May be do you have anny suugestion about it?
@@ -223,7 +245,7 @@ def input_filter_func(point): | |||
node_name, input_filter_func, self._algorithm_key | |||
): | |||
statistics = tensor_collector.get_statistics() | |||
input_fp.extend(statistics.mean_values) | |||
input_fp.extend(Tensor(statistics.mean_values)) |
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 tensor_collector should return already wrapped tensor.
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.
Yes, but i dont want to refactor all in one PR. In this PR fixed ptq for cuda and shows how to use Tensor on real code.
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.
IMO, we should provide the changes in case of UX improvements or developing/code support improvements.
Providing changes only for demonstration
is not good.
So, this is why I still can't understand why should we add a new wrapper for all tensors in this PR.
post_training_quantization/141/ |
post_training_quantization/159 |
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.
One of the major changes that are not clear:
- Why shouldn't we place all tensor-related entities in the one namespace (
Tensor
,functions
); - Why the one algorithm updated with the
Tensor
and the others are not; - Why the developers should use multiple imports to start working with the
Tensor
instead ofimport numpy as np
.
from nncf.experimental.common.tensor_statistics import statistical_functions as s_fns | ||
from nncf.experimental.tensor import Tensor | ||
from nncf.experimental.tensor import functions as fns |
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 still don't get when the fns
and s_fns
would be merged.
04cd1dc
to
35ee9a4
Compare
from nncf.experimental.common.tensor_statistics import statistical_functions as s_fns | ||
from nncf.experimental.tensor import Tensor | ||
from nncf.experimental.tensor import functions as fns |
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.
What is reason to merge them?
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'd like to see the _dispatch_list
improved as directed - after that I don't see major blockers for merging this PR (other than the already visible deficiencies of this approach code-wise as compared to the proper OOP approach)
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.
Please, address my minor comment
Changes
MinMax
andFastBiasCorrection
to use common Tensor.FakeQuantizeParameters
collect data wrapped by tensor.Tensor
:__all__
from function.py, it's works like default behavior.statistical_functions.py
for high level functions that used only function fromfunctions.py
and have no backend specific implementations:Related tickets
113315
Tests