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

DynUNet needs updating for deep_supervision #106

Closed
rijobro opened this issue Jan 13, 2021 · 7 comments · Fixed by #126
Closed

DynUNet needs updating for deep_supervision #106

rijobro opened this issue Jan 13, 2021 · 7 comments · Fixed by #126
Labels
bug Something isn't working

Comments

@rijobro
Copy link
Contributor

rijobro commented Jan 13, 2021

Describe the bug
Following this PR to DynUNet, the corresponding notebook needs updating.

I tried simply removing deep_supervision = True from the constructor, but this wasn't enough.

The subsequent error is attached here: dynunet.txt.

@Nic-Ma I think you might best know what needs to be changed here.

@saruarlive
Copy link

Dear @Nic-Ma @rijobro et al,

The dynunet returns list (containing the output tensor). Had it returned a tensor in validation mode, it would have been easier to use.
So I changed a bit in the sliding window inference code to make it work. What do you think?

  #seg_prob = predictor(window_data, *args, **kwargs).to(device)  # batched patch segmentation
  seg_problist2tensor = predictor(window_data, *args, **kwargs)
  seg_prob = seg_problist2tensor[0] if isinstance(seg_problist2tensor, list) else seg_problist2tensor
  seg_prob = seg_prob.to(device)

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 18, 2021

I will have a try to run the notebook tomorrow, thanks for your issue report.

Thanks.

@rijobro
Copy link
Contributor Author

rijobro commented Jan 18, 2021

Possibly best to make any changes in this PR, as it will already have PEP8 modifications (and git merge is horrible wit jupyter notebooks)

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 19, 2021

Hi @saruarlive and @rijobro ,

I just submitted a commit to fix this issue in that branch: c61cfd1
@wyli , should we try to merge the notebook format PR ASAP? Otherwise, it's hard to develop new PRs and resolve conflicts.

Thanks.

@wyli
Copy link
Contributor

wyli commented Jan 19, 2021

Hi @saruarlive and @rijobro ,

I just submitted a commit to fix this issue in that branch: c61cfd1

@wyli , should we try to merge the notebook format PR ASAP? Otherwise, it's hard to develop new PRs and resolve conflicts.

Thanks.

sure, could you please help review the changes? @Nic-Ma @yiheng-wang-nv @ericspod
the review request is there for 5days #107

@Nic-Ma Nic-Ma added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Jan 19, 2021
@zedtran
Copy link

zedtran commented Jan 19, 2021

Dear @Nic-Ma @rijobro et al,

The dynunet returns list (containing the output tensor). Had it returned a tensor in validation mode, it would have been easier to use.
So I changed a bit in the sliding window inference code to make it work. What do you think?

  #seg_prob = predictor(window_data, *args, **kwargs).to(device)  # batched patch segmentation
  seg_problist2tensor = predictor(window_data, *args, **kwargs)
  seg_prob = seg_problist2tensor[0] if isinstance(seg_problist2tensor, list) else seg_problist2tensor
  seg_prob = seg_prob.to(device)

@saruarlive This was helpful for me. I was finally able to get DynUNet working for the specified tutorial.

@yiheng-wang-nv
Copy link
Contributor

Hi all, I just submitted a PR to update the DynUNet tutorial to meet the changes here. Now, the forward function will only return a single tensor (for both training and eval mode, with or without deep supervision).

@wyli wyli closed this as completed in #126 Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants