Skip to content

2975 torch fgbgtoindices#3038

Merged
Nic-Ma merged 4 commits intoProject-MONAI:devfrom
wyli:2975-torch-fgbgtoindices
Sep 30, 2021
Merged

2975 torch fgbgtoindices#3038
Nic-Ma merged 4 commits intoProject-MONAI:devfrom
wyli:2975-torch-fgbgtoindices

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Sep 28, 2021 •
edited
Loading

Signed-off-by: Wenqi Li [email protected]

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.

wyli added 2 commits September 28, 2021 10:01
@wyli
torch fgbgtoindices
890a0b0 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
unravel indices
65053ea 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli marked this pull request as ready for review September 28, 2021 10:13
@wyli wyli requested review from Nic-Ma and rijobro September 28, 2021 10:13
@Nic-Ma
Copy link
Copy Markdown
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
Copy Markdown
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.

@wyli
simplified
9415e43 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the 2975-torch-fgbgtoindices branch from 0428d66 to 9415e43 Compare September 28, 2021 15:57
@Nic-Ma
Copy link
Copy Markdown
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
Copy Markdown
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:

MONAI/monai/transforms/utils.py

Line 427 in 28fb7d6

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:

MONAI/monai/transforms/croppad/array.py

Line 405 in 28fb7d6

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Nic-Ma commented Sep 28, 2021

Sure, sounds good to me. Investigating.

Thanks.

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Sep 29, 2021

This looks great, thanks!

@Nic-Ma
Copy link
Copy Markdown
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
@Nic-Ma Nic-Ma mentioned this pull request Sep 30, 2021
Merged
7 tasks
@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

Reviewers

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@rijobro rijobro Awaiting requested review from rijobro

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Torch FgBgToIndices

3 participants

@wyli @Nic-Ma @rijobro