Skip to content
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

Feature/sg 631 add registry collate funcs samplers #668

Merged
merged 11 commits into from
Feb 5, 2023

Conversation

eran-deci
Copy link
Contributor

No description provided.

added all_collate_functions.py to training.datasets for registy
@dagshub
Copy link

dagshub bot commented Jan 30, 2023

Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment

Copy link
Collaborator

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment regarding more collate functions left out, inline.

More importantly, the fact you added a registry for collate functions iis great but not sufficient.
Suppose a user registers their collate functions, and we pass some string as an argument to train_dataloader_params. Will that string translate to the right collate function?
The answer is no. You need to resolve the right class by implementing a factory otherwise the registry is useless.
Guidance: see dataloaders.get(), find where we handle the dataloader_params processing and resolve the collate_fn argument with your new CollateFactory.

Copy link
Collaborator

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there, just 2 more comments inline.

shaydeci
shaydeci previously approved these changes Feb 5, 2023
Copy link
Collaborator

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ofrimasad
ofrimasad previously approved these changes Feb 5, 2023
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ofrimasad ofrimasad dismissed stale reviews from shaydeci and themself via 6e96c68 February 5, 2023 08:23
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first code contribution to SG - Good luck!

@eran-deci eran-deci merged commit 61c5371 into master Feb 5, 2023
@eran-deci eran-deci deleted the feature/SG-631_add_registry_collate_funcs_samplers branch February 5, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants