-
Notifications
You must be signed in to change notification settings - Fork 158
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
Support keep_key
in Grouper
?
#256
Comments
Sounds reasonable to me |
For new contributors who would like to work on this issue, we recommend:
|
I took a quick look at this issue.
|
I think that is a reasonable proposal and the API should be cleaner as well, but I feel unsure about doing that for BC and consistency reasons. Ideally, the |
Yes, we should be consistent. But there is no harm to phase out |
Actually In another note, I just added another issue since we had a request to allow
|
Yes :)
That was actually going to be my next question: if we were to add a def merge_fn(key, *args):
return tuple(args) |
Emmm. I agree to add
|
If we choose to add As @ejguan mentioned, My proposal would be to add |
Should we just add Perhaps I'm missing something but it looks like |
Yeah. When |
Is this issue still relevant? The |
@SvenDS9 Thanks for taking interest in contributing TorchData. |
Should I still work on removing the Since |
I think it needs to be done with this issue #334
You can open a PR against PyTorch Core. And, we will review it for sure. |
Summary: As mentioned in #256 it would be useful to have this in all dp that use a `key_fn`. ### Changes - Add keep_key option to `MapKeyZipper` - Test for the new option - Improve example in documentation by using sphinx doctest Pull Request resolved: #1042 Reviewed By: ejguan Differential Revision: D43550124 Pulled By: NivekT fbshipit-source-id: 453418560dd586ebb9ce42dd27eb1bc86eb734d0
Summary: As mentioned in #256 it would be useful to have this in all dp that use a `key_fn`. ### Changes - Add keep_key option to `MapKeyZipper` - Test for the new option - Improve example in documentation by using sphinx doctest Pull Request resolved: #1042 Reviewed By: ejguan Differential Revision: D43550124 Pulled By: NivekT fbshipit-source-id: 453418560dd586ebb9ce42dd27eb1bc86eb734d0
Summary: As mentioned in #256 it would be useful to have this in all dp that use a `key_fn`. ### Changes - Add keep_key option to `MapKeyZipper` - Test for the new option - Improve example in documentation by using sphinx doctest Pull Request resolved: #1042 Reviewed By: ejguan Differential Revision: D43550124 Pulled By: NivekT fbshipit-source-id: 453418560dd586ebb9ce42dd27eb1bc86eb734d0
IterKeyZipper
has an option to keep the key that was zipped on:data/torchdata/datapipes/iter/util/combining.py
Line 53 in 2cf1f20
Is this something we want to support going forward? If yes, it would be nice to have this also on
Grouper
and possibly other similar datapipes. That would come in handy in situations if the key is used multiple times for example if we have aIterKeyZipper
after anGrouper
.Additional Context for New Contributors
See comment below
The text was updated successfully, but these errors were encountered: