Apply func non roundtrippable seq #250
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
|
||
| val = NonRoundtrippableSequence(3) | ||
| result = apply_to_collection(val, int, lambda x: x + 1) | ||
| assert val == result |
There was a problem hiding this comment.
Wouldn't you expect result == [1, 2, 3]? What's a real example where this should become a no-op?
There was a problem hiding this comment.
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.
Sorry, it seems my reply was in pending state for a month and I had to still click on submit review 🤷
| def can_roundtrip_sequence(obj: Sequence) -> bool: | ||
| """Check if sequence can be roundtripped.""" | ||
| try: | ||
| return obj == type(obj)(list(obj)) # type: ignore[call-arg] |
There was a problem hiding this comment.
How can you be sure that list(obj) will work for any sequence? Even though this is covered by the try-except block, the and conditions added to L129 will now skip the is_sequence loop for any sequence that this doesn't consider
Given 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.
Given 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?
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.
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.
|
Closing in favour of upcoming PR implementing an exclude option for user provided types. |
Thank you. Do you mind also having a look at #279? 🙏 |
Before submitting
What does this PR do?
Fixes #249
Note: I'm not sure if the added overhead in the slow path of apply_to_collection due to the shallow sequence copy and the elementwise comparison is undesirable to support this use case. I could reimplement this to cache the result of can_roundtrip_sequence based on the class.
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.
Cheers,
Andreas 😃
📚 Documentation preview 📚: https://lit-utilities--250.org.readthedocs.build/en/250/