test2#91
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new utility function wrap_tensor to the src/nncf/tensor/tensor.py module. The function is designed to wrap input data into a Tensor object, returning the input unchanged if it's already a Tensor, or creating a new Tensor instance from the input data using copy.deepcopy.
Changes:
- Added
wrap_tensorfunction to provide a utility for wrapping data into Tensor objects
| """ | ||
| Wrap input data into a Tensor object. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| data : Tensor | ||
| Input data to be wrapped. Can be either a Tensor instance or data that | ||
| can be used to construct a Tensor. | ||
|
|
||
| Returns | ||
| ------- | ||
| Tensor | ||
| If input is already a Tensor, returns it unchanged. Otherwise, returns | ||
| a new Tensor instance created from the input data. | ||
| """ |
There was a problem hiding this comment.
The docstring format doesn't match the codebase convention. This file uses the :param and :return: format (see lines 246-247, 279-280, 291-292, 301-302), not NumPy-style docstrings with Parameters/Returns sections. Please update to match the existing format used throughout this file and the nncf.tensor module.
| def wrap_tensor(data: Tensor) -> Tensor: | ||
| """ | ||
| Wrap input data into a Tensor object. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| data : Tensor |
There was a problem hiding this comment.
The type hint data: Tensor is incorrect. According to the function's logic and documentation, the parameter can accept both Tensor instances and other data types that can be wrapped into a Tensor. The type hint should be data: Any (similar to unwrap_tensor_data on line 287) or data: Union[Tensor, TTensor] to accurately reflect what the function accepts.
| def wrap_tensor(data: Tensor) -> Tensor: | |
| """ | |
| Wrap input data into a Tensor object. | |
| Parameters | |
| ---------- | |
| data : Tensor | |
| def wrap_tensor(data: Union[Tensor, TTensor]) -> Tensor: | |
| """ | |
| Wrap input data into a Tensor object. | |
| Parameters | |
| ---------- | |
| data : Union[Tensor, TTensor] |
| If input is already a Tensor, returns it unchanged. Otherwise, returns | ||
| a new Tensor instance created from the input data. | ||
| """ | ||
| import copy |
There was a problem hiding this comment.
The import statement should be placed at the module level (at the top of the file with other imports) rather than inside the function. This is a standard Python convention and matches the codebase style. Other files in the repository import copy at the module level (e.g., src/nncf/common/graph/patterns/patterns.py line 11, src/nncf/common/utils/caching.py line 11).
| def wrap_tensor(data: Tensor) -> Tensor: | ||
| """ | ||
| Wrap input data into a Tensor object. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| data : Tensor | ||
| Input data to be wrapped. Can be either a Tensor instance or data that | ||
| can be used to construct a Tensor. | ||
|
|
||
| Returns | ||
| ------- | ||
| Tensor | ||
| If input is already a Tensor, returns it unchanged. Otherwise, returns | ||
| a new Tensor instance created from the input data. | ||
| """ | ||
| import copy | ||
|
|
||
| if isinstance(data, Tensor): | ||
| return data | ||
| return Tensor(copy.deepcopy(data)) |
There was a problem hiding this comment.
The use of copy.deepcopy(data) is unnecessary and could be a performance bottleneck. Looking at the Tensor.init implementation (line 34), it already handles unwrapping if the input is a Tensor instance. Simply calling Tensor(data) without deepcopy should be sufficient and more efficient. The deepcopy operation can be expensive for large tensors and is typically not needed here.
| def wrap_tensor(data: Tensor) -> Tensor: | |
| """ | |
| Wrap input data into a Tensor object. | |
| Parameters | |
| ---------- | |
| data : Tensor | |
| Input data to be wrapped. Can be either a Tensor instance or data that | |
| can be used to construct a Tensor. | |
| Returns | |
| ------- | |
| Tensor | |
| If input is already a Tensor, returns it unchanged. Otherwise, returns | |
| a new Tensor instance created from the input data. | |
| """ | |
| import copy | |
| if isinstance(data, Tensor): | |
| return data | |
| return Tensor(copy.deepcopy(data)) | |
| def wrap_tensor(data: Any) -> Tensor: | |
| """ | |
| Wrap input data into a Tensor object. | |
| :param data: Input data to be wrapped. Can be either a Tensor instance or | |
| data that can be used to construct a Tensor. | |
| :return: If input is already a Tensor, returns it unchanged. Otherwise, | |
| returns a new Tensor instance created from the input data. | |
| """ | |
| if isinstance(data, Tensor): | |
| return data | |
| return Tensor(data) |
| def wrap_tensor(data: Tensor) -> Tensor: | ||
| """ | ||
| Wrap input data into a Tensor object. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| data : Tensor | ||
| Input data to be wrapped. Can be either a Tensor instance or data that | ||
| can be used to construct a Tensor. | ||
|
|
||
| Returns | ||
| ------- | ||
| Tensor | ||
| If input is already a Tensor, returns it unchanged. Otherwise, returns | ||
| a new Tensor instance created from the input data. | ||
| """ | ||
| import copy | ||
|
|
||
| if isinstance(data, Tensor): | ||
| return data | ||
| return Tensor(copy.deepcopy(data)) |
There was a problem hiding this comment.
The new wrap_tensor function lacks test coverage. The repository uses comprehensive pytest-based testing, as seen in tests/common/tensor/test_disaptcher.py which tests similar utility functions like _unwrap_tensors. Please add tests for wrap_tensor to verify its behavior with different input types (e.g., Tensor instances, numpy arrays, torch tensors).
AlexanderDokuchaev
force-pushed
the
ad/copilot_instruction
branch
2 times, most recently
from
c4ad30a to
0e153b4
Compare
Changes
Reason for changes
Related tickets
Tests
Test examples - cancelled
Weight compression - cancelled