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.

Sign up for GitHub

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

Jump to bottom

[OTX] consider multi GPU when setting drop last #1520

Merged
merged 3 commits into from
Jan 13, 2023
Merged

[OTX] consider multi GPU when setting drop last #1520

merged 3 commits into from
Jan 13, 2023

Conversation

eunwoosh
Copy link
Contributor

@eunwoosh eunwoosh commented Jan 12, 2023 •
edited
Loading

Summray

  • setting drop_laste considering multi GPU

Detail

Although samplers_per_gpu * num_gpus is bigger than number of images, drop_last is set as 0, then max_iter is calculated as 0, which makes a bug.

@eunwoosh eunwoosh requested a review from a team as a code owner January 12, 2023 13:46
@github-actions github-actions bot added ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests labels Jan 12, 2023
JihwanEom
JihwanEom previously approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@JihwanEom JihwanEom left a comment

Choose a reason for hiding this comment

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

LGTM. Let's merge after passing TCs.

otx/algorithms/classification/tasks/inference.py Outdated Show resolved Hide resolved
otx/mpa/cls/trainer.py Outdated Show resolved Hide resolved
@eunwoosh
Copy link
Contributor Author

prmerge pass
All CI tests are passed.

@eunwoosh
Copy link
Contributor Author

As you can see all CI tests are passed and I just changed variable names from num_gpus to num_worlds.
Could you approve if you think it's okay?

@eunwoosh eunwoosh requested a review from JihwanEom January 13, 2023 00:22
sungmanc
sungmanc approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@sungmanc sungmanc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Don't we need to consider detection/segmentation tasks?

@eunwoosh
Copy link
Contributor Author

Don't we need to consider detection/segmentation tasks?

Yes, I checked that there is code to set it as True, but only classification task has.

@eunwoosh eunwoosh merged commit b509c11 into feature/otx Jan 13, 2023
@eunwoosh eunwoosh deleted the es/drop_last_consider_multigpu branch January 13, 2023 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sungmanc sungmanc sungmanc approved these changes

@JihwanEom JihwanEom JihwanEom approved these changes

@goodsong81 goodsong81 Awaiting requested review from goodsong81

@supersoob supersoob Awaiting requested review from supersoob

@harimkang harimkang Awaiting requested review from harimkang

@jaegukhyun jaegukhyun Awaiting requested review from jaegukhyun

Assignees
No one assigned
Labels
ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eunwoosh @JihwanEom @sungmanc