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

[Enhance] In browse dataset : CombinedDataset element are now browse in turn, and image saved into their dataset name folder #2985

Merged

Conversation

drazicmartin
Copy link
Contributor

@drazicmartin drazicmartin commented Mar 14, 2024

Motivation

Make it simpler to see the first N images of each dataset when using CombinedDataset.
Also put images in an appropriate folder named by their dataset name.

Modification

  • in browse_dataset.py
    • added the argument --max-item-per-dataset
    • put images in a folder named by the dataset the images belongs to
    • if using CombinedDataset, browse each dataset by taking one element from each dataset in turn.
  • docs in prepare_datasets.md

Use cases (Optional)

for first 50 images of each dataset

python tools/misc/browse_dataset.py --max-item-per-dataset 50 [...]

for every images in each dataset

python tools/misc/browse_dataset.py --max-item-per-dataset 999999 [...]

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit tests to ensure correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • CLA has been signed and all committers have signed the CLA in this PR.

progress_bar = mmengine.ProgressBar(len(dataset))
if isinstance(dataset, CombinedDataset):
# Get indexes to traverse each dataset element in turn.
max_length = min(min(dataset._lens), args.max_item_per_dataset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. It is not recommended to use private attributes outside the class. Instead, it is recommended to add a new property for the CombinedDataset class.
  2. There is no need to enforce a uniform number of items across datasets. Instead, the number of items per dataset can be dynamically determined as min(len(subdataset), args.max_item_per_dataset), aligning with the intended behavior of the max_item_per_dataset argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, Thx for commenting !

I added :

  • A public property in CombinedDataset that return _lens
  • Reformated to take account of your second point

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 71.72%. Comparing base (108691a) to head (ffbee78).
Report is 3 commits behind head on dev-1.x.

Files Patch % Lines
mmpose/datasets/dataset_wrappers.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           dev-1.x    #2985      +/-   ##
===========================================
+ Coverage    71.68%   71.72%   +0.03%     
===========================================
  Files          293      293              
  Lines        20072    20074       +2     
  Branches      3408     3408              
===========================================
+ Hits         14389    14398       +9     
+ Misses        4865     4860       -5     
+ Partials       818      816       -2     
Flag Coverage Δ
unittests 71.72% <50.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ben-Louis Ben-Louis merged commit de67839 into open-mmlab:dev-1.x Mar 20, 2024
15 of 17 checks passed
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