-
Notifications
You must be signed in to change notification settings - Fork 57
Features: TakeProperties #400
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
base: develop
Are you sure you want to change the base?
Conversation
* add docs of /features/__lt__ * u --------- Co-authored-by: Jiacheng Huang <[email protected]>
* add docs of /features/__rlt__ * u * u * Update features.py --------- Co-authored-by: Jiacheng Huang <[email protected]> Co-authored-by: Giovanni Volpe <[email protected]>
* add docs 0f /featuers/__le__ * u --------- Co-authored-by: Jiacheng Huang <[email protected]>
* add docs of /features/__rle__ * u * u * Update features.py --------- Co-authored-by: Jiacheng Huang <[email protected]> Co-authored-by: Giovanni Volpe <[email protected]>
Co-authored-by: Jiacheng Huang <[email protected]>
* add docs 0f /features/__rge__ * u * u * right , * Update features.py --------- Co-authored-by: Jiacheng Huang <[email protected]> Co-authored-by: Giovanni Volpe <[email protected]>
* add docs of /features/__xor__ * remove blank spaces --------- Co-authored-by: Jiacheng Huang <[email protected]>
* add docs of /features/__rand__ * Update features.py --------- Co-authored-by: Jiacheng Huang <[email protected]> Co-authored-by: Giovanni Volpe <[email protected]>
Co-authored-by: Jiacheng Huang <[email protected]>
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 that the PR looks quite good, and the unittesting is good. You should check what is written in the docs and type hints, because to me it isn't clear from that text if it can also return torch tensors. And I also wrote some comments about a few smaller things
@@ -9462,18 +9462,18 @@ class TakeProperties(Feature): | |||
Indicates whether this feature distributes computation across inputs. | |||
Always `False` for `TakeProperties`, as it processes sequentially. | |||
__list_merge_strategy__: int | |||
Specifies how lists of properties are merged. Set to | |||
Specifies how lists of properties are merged. Set to | |||
`MERGE_STRATEGY_APPEND` to append values to the result list. | |||
|
|||
Methods | |||
------- | |||
`get(image: Any, names: tuple[str, ...], **kwargs: dict[str, Any]) -> np.ndarray | tuple[np.ndarray, ...]` |
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.
This line is too long
image: Any, | ||
names: tuple[str, ...], | ||
_ID: tuple[int, ...] = (), | ||
**kwargs: Any, | ||
) -> np.ndarray | tuple[np.ndarray, ...]: | ||
) -> NDArray | tuple[NDArray, torch.Tensor, ...]: | ||
"""Extract the specified properties from the feature pipeline. | ||
|
||
This method retrieves the values of the specified properties from the |
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.
There is one extra blank space at the end of the line
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.
This is also the case for line 9545
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.
And some more lines. So please double check this :)
properties.Property(torch.tensor(42.123))) | ||
|
||
take_properties = features.TakeProperties(feature) | ||
take_properties = features.TakeProperties(feature) |
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.
Is there a reason for writing this twice after each other? It is the same for the numpy test
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.
Oh, this is something historic, I just copied and pasted from the previous numpy test. I will check and clean them,
unittest, torch compatible, docs