test#90
Conversation
AlexanderDokuchaev
requested review from
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR adds a new wrap_tensor utility function to the tensor module with minimal context provided in the PR description (title is simply "test" with an empty template).
Changes:
- Added
wrap_tensorfunction tosrc/nncf/tensor/tensor.pythat wraps input data into a Tensor object, returning existing Tensor instances unchanged or creating new ones with deep-copied data
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 documentation style uses NumPy format (Parameters/Returns sections with dashes), while the rest of the file uses reStructuredText format with :param and :return tags (see lines 243-252, 276-281, 288-293, 299-302). The documentation should be consistent with the existing convention in this file.
| 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. | |
| :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. |
| 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.
This function appears to be redundant. The Tensor class constructor (line 34) already handles both Tensor instances and raw data, extracting the .data attribute when a Tensor is passed. Simply calling Tensor(data) would achieve the same result without needing a separate wrapper function. If this function is intended to perform a deep copy, that intent should be documented and the implementation should be reconsidered given the issues with deepcopy on tensor backends.
| 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 top of the file with other imports, not inside the function. The codebase follows the convention of placing all imports at module level (lines 11-23).
| 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 specifies data: Tensor but the documentation states it can be "either a Tensor instance or data that can be used to construct a Tensor". The type hint should be data: Union[Tensor, TTensor] or data: Any to match the intended behavior described in the docstring.
| 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] |
| 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 is unnecessary and potentially problematic. The Tensor.__init__ method (line 34) already handles both Tensor instances and raw data correctly by extracting .data from Tensor objects. Additionally, deepcopy can be expensive for large tensors and may not work correctly with all tensor backends (e.g., CUDA tensors). Consider simply using return Tensor(data) for non-Tensor inputs.
| import copy | |
| if isinstance(data, Tensor): | |
| return data | |
| return Tensor(copy.deepcopy(data)) | |
| if isinstance(data, Tensor): | |
| return data | |
| return Tensor(data) |
Changes
Reason for changes
Related tickets
Tests