Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[REVIEW] Implement Feature Request from #1077 on Left Padding #1126
[REVIEW] Implement Feature Request from #1077 on Left Padding #1126
Changes from 8 commits
a52a90c
ed61663
ff1e396
d9e457d
e25c6e8
295d4e2
5166d57
e55336c
af8aa57
299d356
1285783
364bcf1
071b8bf
3cce162
cebb715
5acd76a
1684289
a319501
0be389e
d93f9c5
0c0ce69
941d2f3
7944b2a
76c0024
46847cb
c7ae873
d86cec3
d90e1df
b21c57d
2150ede
a51aa44
01749f9
b305afa
2febf1a
587ef0c
dd9927e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is this padding value supposed to be passed by the user? It seems like this parameter is only set on a non-public method - and isn't set anywhere
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.
Thank you Ben for your review. Yes, let me see how to have this option be user-accessible.
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 will plan to implement these options as user-accessible argument in the signatures for the
TorchAsyncItr
class intorch.py
and in theKerasSequenceLoader
class intensorflow.py
, if there are not objections to this.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.
@benfred I have seen the test files for the dataloaders here. Based on your knowledge of the existing tests, are there some existing tests that you would advise or guide me that I can use as a template to test this padding feature for the two dataloader implementations?
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.
@benfred Would you have any thoughts or advice on how to implement a user-facing interface to this option? Gabriel had suggested to modify a couple of the private methods in this torch dataloader module. I do not see anywhere though that either of these private methods,
_build_sparse_tensor()
or_get_indices()
are used in thistorch
module or anywhere else in the codebase. My guess is that he had either wanted to call these private methods directly, or was mistaken where to implement this feature. Would you have any advice or guidance on whether to leave this implementation in the private methods or where to expose the padding side argument to users?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.
Should this fail here? Shouldn't we handle this by default?
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.
From the original issue from Gabriel, it sounded to me, based on what was written there, that right padding has already been implemented. In the description for issue 1077, there is for instance:
The PyT and TF Dataloader support padding list (sparse) features to the right, which means that shorter list sequences will be completed with 0s in the right.
I did not want to reduplicate it here to avoid doing the same thing in multiple places of the codebase. Let me investigate some more whether this has been already implemented, and hence should not be duplicated, or whether it makes sense to implement this feature 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.
@benfred Would you know off-hand, based on your knowledge of the codebase, whether right padding has definitely been or not been implemented elsewhere in the repository?