-
Notifications
You must be signed in to change notification settings - Fork 16
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
Apply func non roundtrippable seq #250
Changes from 4 commits
e9b8178
42b0ea2
4c14e62
2d58e0e
61d50cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,3 +359,13 @@ class Foo: | |
foo = Foo(0) | ||
result = apply_to_collection(foo, int, lambda x: x + 1, allow_frozen=True) | ||
assert foo == result | ||
|
||
|
||
def test_apply_to_collection_non_roundtrippable_sequence(): | ||
class NonRoundtrippableSequence(list): | ||
def __init__(self, x: int): | ||
super().__init__(range(int(x))) | ||
|
||
val = NonRoundtrippableSequence(3) | ||
result = apply_to_collection(val, int, lambda x: x + 1) | ||
assert val == result | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't you expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This came up when using: https://github.com/Bayer-Group/pado/blob/635d7b8b57e527254d6302730100a6dab5a2095f/pado/images/ids.py#L126-L351 Where ImageId instances are tuple subclasses but they don't roundtrip, i.e.: iid = ImageId("a", "b", "c", site="site-1")
ImageId(list(iid)) <- is not allowed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc: @ap-- ^^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, it seems my reply was in pending state for a month and I had to still click on submit review 🤷 |
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.
How can you be sure that
list(obj)
will work for any sequence? Even though this is covered by the try-except block, theand
conditions added to L129 will now skip the is_sequence loop for any sequence that this doesn't considerGiven the complexity and that your data structure is very specific to your problem. Could we instead expose an API so that you can have your data structure define whether it should be treated as a sequence or not?
For instance, that is similar to how pytrees are implemented which are much more extensible and performant: https://github.com/pytorch/pytorch/blob/main/torch/utils/_pytree.py#L163. In this case, it could also be an attribute of the data structure
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.
That sounds like the best solution. An exclude list for types that should just be passed through would be ideal. I can work on a PR in early June.
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.
so how is it going here? :)
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.
It's still on the list. I'm just lacking time to work on open-source these days...
I'll close this PR and will open a new one with exlude support when I find the time.