Skip to content

5386 Update HoverNet structure#5405

Merged
bhashemian merged 26 commits intoProject-MONAI:devfrom
yiheng-wang-nv:5386-modify-hovernet-and-add-pretrain
Nov 8, 2022
Merged

5386 Update HoverNet structure#5405
bhashemian merged 26 commits intoProject-MONAI:devfrom
yiheng-wang-nv:5386-modify-hovernet-and-add-pretrain

Conversation

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor

@yiheng-wang-nv yiheng-wang-nv commented Oct 26, 2022 •
edited
Loading

Signed-off-by: Yiheng Wang [email protected]

Fixes #5386 #5387 .

Description

This PR modifies HoverNet via change the kernel size and padding size in order to be consistent with the reference source.

This PR also enables to output same shape as the input when using fast mode (takes the conic branch version (see: https://github.com/vqdang/hover_net/tree/conic) for reference).

What's more, the functionality of loading pretrained weights is added, users need to add the url which contains the weights that follow the format of the encoder.

Finally, this PR adds the np_out_channels arg to control the output channels of the nucleus prediction branch.

Types of changes

  • Breaking change (fix or new feature that would cause existing functionality to change).
  • 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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

yiheng-wang-nv commented Oct 26, 2022

The changed HoverNet is still not 100% the same as the reference version on the encoder side, since we use conv layers to do the padding but the referred version defines TFSamepaddingLayer (see here) and employ it before the conv layer.

Hi @JHancox , could you please share some experiences of the difference here? In my point of view, it's more convenient to use your method since we do not need to maintain more layers within the network.

@yiheng-wang-nv yiheng-wang-nv requested review from Nic-Ma and wyli October 26, 2022 06:44
@yiheng-wang-nv
fix black
07b60a8 
Signed-off-by: Yiheng Wang <[email protected]>
@yiheng-wang-nv
Merge branch '5386-modify-hovernet-and-add-pretrain' of github.com:yi…
4624443 
…heng-wang-nv/MONAI into 5386-modify-hovernet-and-add-pretrain
@yiheng-wang-nv
add freeze encoder func
ab7f3f3 
Signed-off-by: Yiheng Wang <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Oct 26, 2022

Hi @KumoLiu ,

Could you please help verify the PR in your draft training pipeline?
If everything works fine, then let's invite more reviewers.

Thanks in advance.

@Nic-Ma Nic-Ma requested review from KumoLiu and bhashemian October 26, 2022 07:38
wyli
wyli reviewed Oct 26, 2022
View reviewed changes
@JHancox
Copy link
Copy Markdown
Contributor

JHancox commented Oct 26, 2022

The changed HoverNet is still not 100% the same as the reference version on the encoder side, since we use conv layers to do the padding but the referred version defines TFSamepaddingLayer (see here) and employ it before the conv layer.

Hi @JHancox , could you please share some experiences of the difference here? In my point of view, it's more convenient to use your method since we do not need to maintain more layers within the network.

Thanks @yiheng-wang-nv. In their PyTorch code, they tried to emulate their original Tensorflow code, which used Tensorflow's simpler padding/cropping implementation. As you say, this resulted in additional layers that I felt could be eliminated in a more elegant way. So, although there are differences, it's the consistent output that is important.

JHancox
JHancox reviewed Oct 26, 2022
View reviewed changes
bhashemian
bhashemian reviewed Oct 26, 2022
View reviewed changes
@bhashemian
Copy link
Copy Markdown
Member

bhashemian commented Oct 31, 2022

@JHancox will give @yiheng-wang-nv new unittests by Nov 1 to be added to this PR.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

yiheng-wang-nv commented Oct 31, 2022

@JHancox will give @yiheng-wang-nv new unittests by Nov 1 to be added to this PR.

Thanks @JHancox @drbeh . I already received the code of unit tests from Jonny, I will update them into this PR soon.

KumoLiu
KumoLiu reviewed Nov 1, 2022
View reviewed changes
KumoLiu
KumoLiu reviewed Nov 1, 2022
View reviewed changes
@KumoLiu
Copy link
Copy Markdown
Contributor

KumoLiu commented Nov 4, 2022

Also for the output channel in NP branch, it seems to be hard-coded in 2. Maybe we should make it not hard-code here?

MONAI/monai/networks/nets/hovernet.py

Lines 520 to 523 in 52525a8

self.nucleus_prediction = _DecoderBranch(kernel_size=_ksize, same_padding=decoder_padding)
self.horizontal_vertical = _DecoderBranch(kernel_size=_ksize, same_padding=decoder_padding)
self.type_prediction: Optional[_DecoderBranch] = (
_DecoderBranch(out_channels=out_classes, kernel_size=_ksize, same_padding=decoder_padding)

Thanks!

@JHancox
Copy link
Copy Markdown
Contributor

JHancox commented Nov 4, 2022

Hi @KumoLiu

Hi @yiheng-wang-nv @JHancox @drbeh, maybe we should remove this line in the network to make it more flexible?

  • I think that's a valid point. It would make more sense to be in the training pipeline

Also for the output channel in NP branch, it seems to be hard-coded in 2. Maybe we should make it not hard-code here?

  • Are you talking about the number of channels as 2, the number of branches or something else?

@KumoLiu
Copy link
Copy Markdown
Contributor

KumoLiu commented Nov 4, 2022 •
edited
Loading

Hi @JHancox

  • Are you talking about the number of channels as 2, the number of branches or something else?

Yes, I am talking about the number of channels in the NP branch. I think it can be not only 2, but also 1 or other values.

Thanks!

@JHancox
Copy link
Copy Markdown
Contributor

JHancox commented Nov 4, 2022

Hi @JHancox

  • Are you talking about the number of channels as 2, the number of branches or something else?

Yes, I am talking about the number of channels in the NP branch. I think it can be not only 2, but also 1 or other values.

Thanks!

I don't think that there is a need to generalize this, because the purpose of the branch is pretty specific. However, as you mentioned previously, there is a good argument to simply make it a 1 channel output and then apply a sigmoid to the output, rather than having 2 channels with softmax.

@bhashemian
Copy link
Copy Markdown
Member

bhashemian commented Nov 4, 2022

Hi @JHancox

  • Are you talking about the number of channels as 2, the number of branches or something else?

Yes, I am talking about the number of channels in the NP branch. I think it can be not only 2, but also 1 or other values.
Thanks!

I don't think that there is a need to generalize this, because the purpose of the branch is pretty specific. However, as you mentioned previously, there is a good argument to simply make it a 1 channel output and then apply a sigmoid to the output, rather than having 2 channels with softmax.

@KumoLiu @yiheng-wang-nv, I agree with @JHancox here since we already have another branch (NC) that classify further this prediction into subclasses, so the purpose of NP would be a binary prediction (unlike other use cases that we might have multi-class segmentation as one output).

@KumoLiu
Copy link
Copy Markdown
Contributor

KumoLiu commented Nov 4, 2022

Hi @JHancox

  • Are you talking about the number of channels as 2, the number of branches or something else?

Yes, I am talking about the number of channels in the NP branch. I think it can be not only 2, but also 1 or other values.
Thanks!

I don't think that there is a need to generalize this, because the purpose of the branch is pretty specific. However, as you mentioned previously, there is a good argument to simply make it a 1 channel output and then apply a sigmoid to the output, rather than having 2 channels with softmax.

Even if there is only a choice of 1 or 2, maybe we can also make it not hard-code here?
@yiheng-wang-nv, @drbeh, @Nic-Ma, @wyli, what do you think?

@bhashemian
Copy link
Copy Markdown
Member

bhashemian commented Nov 4, 2022

@KumoLiu, having 2 classes where they are mutually exclusive is identical to having one class. I believe it has been a mistake that the original authors have used 2 classes.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

yiheng-wang-nv commented Nov 7, 2022

Hi @JHancox

  • Are you talking about the number of channels as 2, the number of branches or something else?

Yes, I am talking about the number of channels in the NP branch. I think it can be not only 2, but also 1 or other values.
Thanks!

I don't think that there is a need to generalize this, because the purpose of the branch is pretty specific. However, as you mentioned previously, there is a good argument to simply make it a 1 channel output and then apply a sigmoid to the output, rather than having 2 channels with softmax.

Even if there is only a choice of 1 or 2, maybe we can also make it not hard-code here? @yiheng-wang-nv, @drbeh, @Nic-Ma, @wyli, what do you think?

Hi, I think add an arg to control the output channel of NP branch makes sense, for reproducing the original result, just need to keep the default setting.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

yiheng-wang-nv commented Nov 7, 2022 •
edited
Loading

Update: according to the latest discussion, the current version will be updated and the load pretrained weights part will be removed due to the license issue.

@yiheng-wang-nv yiheng-wang-nv changed the title 5386 Modify HoverNet and add pretrained weights support 5386 Update HoverNet structure Nov 7, 2022
KumoLiu
KumoLiu approved these changes Nov 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

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

Thanks for the update, looks good to me now!

bhashemian
bhashemian reviewed Nov 7, 2022
View reviewed changes
@bhashemian bhashemian enabled auto-merge (squash) November 7, 2022 21:08
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Nov 7, 2022

/build

1 similar comment
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Nov 7, 2022

/build

@bhashemian bhashemian merged commit 044295e into Project-MONAI:dev Nov 8, 2022
@yiheng-wang-nv yiheng-wang-nv deleted the 5386-modify-hovernet-and-add-pretrain branch November 8, 2022 03:29
bhashemian added a commit to bhashemian/MONAI that referenced this pull request Nov 23, 2022
@yiheng-wang-nv @bhashemian
5386 Update HoverNet structure (Project-MONAI#5405)
58e04e9 
Signed-off-by: Yiheng Wang <[email protected]>

Fixes Project-MONAI#5386 Project-MONAI#5387 .

### Description

This PR modifies HoverNet via change the kernel size and padding size in
order to be consistent with the reference source.

This PR also enables to output same shape as the input when using fast
mode (takes the conic branch version (see:
https://github.com/vqdang/hover_net/tree/conic) for reference).

What's more, the functionality of loading pretrained weights is added,
users need to add the url which contains the weights that follow the
format of the encoder.

Finally, this PR adds the `np_out_channels` arg to control the output
channels of the nucleus prediction branch.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] 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 --disttests`.
- [x] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: Yiheng Wang <[email protected]>
Co-authored-by: Behrooz Hashemian <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli left review comments

@bhashemian bhashemian bhashemian approved these changes

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

@JHancox JHancox JHancox approved these changes

@KumoLiu KumoLiu KumoLiu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent places of HoverNet

6 participants

@yiheng-wang-nv @Nic-Ma @JHancox @bhashemian @KumoLiu @wyli