Skip to content

Conversation

@yuchongzhang
Copy link
Collaborator

PR Type

Refactoring

Short Description

For examples that use CIFAR-10, I simply moved the code for loading data to utils/load_data.py to reduce code duplication a bit, since it is also where the code for loading MNIST is located.

I considered writing a function for loading datasets in general, but that would require the client code to provide dataset-specific information which I thought might not be desirable.

Tests Added

...

return train_loader, validation_loader, num_examples


def load_cifar10_data(data_dir: Path, batch_size: int) -> Tuple[DataLoader, DataLoader, Dict[str, int]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: we can get a sampler object here and subsample cifar10 as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like Fatemeh's suggestion here. You could make the samplers Optional[LabelBasedSampler] in both load MNIST and CIFAR methods that default to none.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the function to include a sampler argument.

Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Looks good to go from my perspective. I'd double check with Fatemeh that this is what she had in mind.

@emersodb emersodb requested a review from fatemetkl May 30, 2023 17:53
Copy link
Collaborator

@fatemetkl fatemetkl left a comment

Choose a reason for hiding this comment

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

Looks good!

@yuchongzhang yuchongzhang merged commit fc947e6 into main May 30, 2023
@yuchongzhang yuchongzhang deleted the example-refactor branch May 30, 2023 19:43
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.

4 participants