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

2975 torch fgbgtoindices #3038

Merged
merged 4 commits into from
Sep 30, 2021
Merged

Conversation

wyli
Copy link
Contributor

@wyli wyli commented Sep 28, 2021

Signed-off-by: Wenqi Li wenqil@nvidia.com

Fixes #2975

Description

  • unravel_index is still in numpy, otherwise it supports both backends added both backends

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli marked this pull request as ready for review September 28, 2021 10:13
@wyli wyli requested review from rijobro and Nic-Ma September 28, 2021 10:13
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 28, 2021

pending the PR, I will test with spleen fast training and get back ASAP.

Thanks.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 28, 2021

Hi @rijobro ,

BTW, have you implemented this feature locally in your branch? If yes, maybe you can share some comments here.

Thanks.

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the 2975-torch-fgbgtoindices branch from 0428d66 to 9415e43 Compare September 28, 2021 15:57
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 28, 2021

Mark: training time with this PR around 60s, and without this PR 56-58s.

Thanks.

@wyli
Copy link
Contributor Author

wyli commented Sep 28, 2021

Mark: training time with this PR around 60s, and without this PR 56-58s.

Thanks.

I looked into this issue, the root cause is that
coordinates were (1) converted into list of numbers:

corrected_centers: List[int] = [c.item() if isinstance(c, torch.Tensor) else c for c in centers] # type: ignore

and (2) the numbers are converted into tensors:
roi_center = torch.as_tensor(roi_center, dtype=torch.int16)

Previously fg_indices, bg_indices were numpy arrays, so there's no step (1) conversion.

So I think the issue is not really in this PR, it's from the generate_pos_neg_label_crop_centers and SpatialCrop of RandCropByPosNegLabeld.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 28, 2021

Hi @wyli ,

OK, thanks for your investigation!
Let me try to fix the issue in a separate PR ASAP.

Thanks.

@wyli
Copy link
Contributor Author

wyli commented Sep 28, 2021

Hi @wyli ,

OK, thanks for your investigation! Let me try to fix the issue in a separate PR ASAP.

Thanks.

thanks! please help fix the issue and we can merge this PR after that

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 28, 2021

Sure, sounds good to me. Investigating.

Thanks.

@rijobro
Copy link
Contributor

rijobro commented Sep 29, 2021

This looks great, thanks!

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Sep 30, 2021

I think we already found the root cause of the performance issue, no need to put the fg / bg indices on GPU as we actually need to move it to CPU and index image.
So I will merge this PR first, then create another PR to optimize the logic.

Thanks.

@Nic-Ma Nic-Ma merged commit f909a6b into Project-MONAI:dev Sep 30, 2021
@wyli wyli deleted the 2975-torch-fgbgtoindices branch November 18, 2021 22:18
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.

Torch FgBgToIndices
3 participants