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

[GSK-2355, GSK-2349, GSK-2373] DataLoader structure #5

Merged
merged 24 commits into from
Dec 13, 2023

Conversation

Hartorn
Copy link
Member

@Hartorn Hartorn commented Dec 8, 2023

No description provided.

@Hartorn Hartorn self-assigned this Dec 8, 2023
@rabah-khalek rabah-khalek mentioned this pull request Dec 9, 2023
@rabah-khalek rabah-khalek changed the title Try to use dataset differently [GSK-2355] DataLoader structure Dec 9, 2023
Copy link

linear bot commented Dec 9, 2023

@rabah-khalek rabah-khalek changed the title [GSK-2355] DataLoader structure [GSK-2355, GSK-2349] DataLoader structure Dec 9, 2023
Copy link

linear bot commented Dec 9, 2023

GSK-2349 Caching

Ideas:

@Hartorn Hartorn marked this pull request as ready for review December 10, 2023 13:19
@Hartorn Hartorn marked this pull request as draft December 10, 2023 13:19
Copy link
Contributor

@rabah-khalek rabah-khalek left a comment

Choose a reason for hiding this comment

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

Overall great work @Hartorn!

I had doubts about promoting the class-based (as opposed to a method-based) approach to define augmented-datasets (and soon sliced-datasets) but atm I can't see any downsides as the class-based one allows for more customisability and flexibility.

I mainly did some refactoring and conflict resolutions with main.
In particular:

  • I removed facial_part as a parameter to model.predict() as this should be dictated by the passed dataset.
  • I renamed the base classes into:
    • DataIteratorBase: The base class for all dataloader classes serving as skeleton
    • DataLoaderBase: The base class for all dataloader classes that implement loading methods
    • DataLoaderWrapper: The base class for all dataloader classes that serve as wrappers.

I organised the spin-off like DataLoader300W into loaders and CroppedDataLoader into wrappers.

To me it was the clearest way for now, let me know if you have other ideas.

TODO:

  • Could you please remove the introduced ground_truths from PredictionResult? Even-though it might make things easier in some cases, but we should avoid mixing data-related attributes with model ones.
  • clean up commented code + write docstrings
  • I can take care of ensuring that all notebooks are updated before we merge.

TODO In a different PR:

loreal_poc/models/base.py Outdated Show resolved Hide resolved
@Hartorn Hartorn marked this pull request as ready for review December 12, 2023 08:48
@Hartorn Hartorn marked this pull request as draft December 12, 2023 16:03
Copy link
Contributor

@rabah-khalek rabah-khalek left a comment

Choose a reason for hiding this comment

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

More or less minor comments

loreal_poc/models/base.py Outdated Show resolved Hide resolved
loreal_poc/tests/base.py Show resolved Hide resolved
loreal_poc/tests/base.py Outdated Show resolved Hide resolved
loreal_poc/tests/base.py Outdated Show resolved Hide resolved
@rabah-khalek rabah-khalek marked this pull request as ready for review December 13, 2023 12:34
@rabah-khalek rabah-khalek changed the title [GSK-2355, GSK-2349] DataLoader structure [GSK-2355, GSK-2349, GSK-2373] DataLoader structure Dec 13, 2023
Copy link

linear bot commented Dec 13, 2023

GSK-2373 meta data handling in dataloader

  • returning conform output from dataloader
  • meta is a dict()
return res_img, res_marks, meta 
  • res_marks, meta are optional

@rabah-khalek rabah-khalek merged commit 0f76a9f into main Dec 13, 2023
1 of 3 checks passed
@rabah-khalek rabah-khalek deleted the dataset-first branch December 13, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants