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

MIL Component - MILModel #3236

Merged
merged 11 commits into from
Nov 16, 2021
Merged

MIL Component - MILModel #3236

merged 11 commits into from
Nov 16, 2021

Conversation

myron
Copy link
Collaborator

@myron myron commented Nov 2, 2021

Fix #3163

A wrapper around backbone CNN classification model suitable for MIL

@myron myron added this to the Multiple-instance Learning [P0] milestone Nov 2, 2021
@myron myron self-assigned this Nov 2, 2021
@bhashemian bhashemian self-requested a review November 2, 2021 00:02
Copy link
Member

@bhashemian bhashemian left a comment

Choose a reason for hiding this comment

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

Hi @myron, all your git commits need to be signed for MONAI. You should use -s in ALL your commits:

git commit -sm "Add commit message"

https://github.com/Project-MONAI/MONAI/pull/3236/checks?check_run_id=4074346048

@myron
Copy link
Collaborator Author

myron commented Nov 2, 2021

Hi @myron, all your git commits need to be signed for MONAI. You should use -s in ALL your commits:

git commit -sm "Add commit message"

https://github.com/Project-MONAI/MONAI/pull/3236/checks?check_run_id=4074346048

even for draft PRs ? I thought this is just to have an initial discussion

@myron
Copy link
Collaborator Author

myron commented Nov 2, 2021

Ok, signed and pushed

@bhashemian
Copy link
Member

Thank you @myron. The draft PR is the work in progress to let the repo maintainer know what is being developed. So you can continue to improve it until it is ready to review (and passing all the tests)

I will try to give my comments earlier but please work on addressing the tests that are not passing. You can start from the easy one like code formatting. Please let me know if you have any question.

@myron myron force-pushed the milmodel branch 2 times, most recently from 134fbbe to af988b8 Compare November 2, 2021 00:41
@Nic-Ma Nic-Ma requested review from wyli and yiheng-wang-nv November 2, 2021 02:04
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 2, 2021

Hi @myron ,

Please add guys in reviewers instead of assignees.

Thanks.

monai/networks/nets/milmodel.py Show resolved Hide resolved
monai/networks/nets/milmodel.py Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
@myron myron force-pushed the milmodel branch 2 times, most recently from 1b4a429 to 50dc0c6 Compare November 3, 2021 01:50
@myron
Copy link
Collaborator Author

myron commented Nov 3, 2021

I've updated this model, added enum and moved some options into arguments
@drbeh does it look better?

monai/networks/nets/milmodel.py Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
@myron myron changed the title [DLMED] MILModel draft PR MIL Component - MILModel draft PR Nov 4, 2021
Signed-off-by: myron <amyronenko@nvidia.com>
Signed-off-by: myron <amyronenko@nvidia.com>
Signed-off-by: myron <amyronenko@nvidia.com>
Signed-off-by: myron <amyronenko@nvidia.com>
@myron
Copy link
Collaborator Author

myron commented Nov 12, 2021

I ran into another JIT issue, it fails the test on the older Pytorch version (1.7, over a year old), it looks like at that time Enum wasn't supported in JIT.

@wyli @Nic-Ma any suggestions?
we can remove Enum, and use strings for MilMode, or we can skip this test ?

thanks

RuntimeError:
Module 'MILModel' has no attribute 'mil_mode' (This attribute exists on the Python module, but we failed to convert Python type: 'monai.networks.nets.milmodel.MilMode' to a TorchScript type.):
  File "/mnt/amproj/Code/MONAI/monai/networks/nets/milmodel.py", line 161
        sh = x.shape

        if self.mil_mode == MilMode.MEAN:


@bhashemian
Copy link
Member

I ran into another JIT issue, it fails the test on the older Pytorch version (1.7, over a year old), it looks like at that time Enum wasn't supported in JIT.

@wyli @Nic-Ma any suggestions? we can remove Enum, and use strings for MilMode, or we can skip this test ?

thanks

RuntimeError:
Module 'MILModel' has no attribute 'mil_mode' (This attribute exists on the Python module, but we failed to convert Python type: 'monai.networks.nets.milmodel.MilMode' to a TorchScript type.):
  File "/mnt/amproj/Code/MONAI/monai/networks/nets/milmodel.py", line 161
        sh = x.shape

        if self.mil_mode == MilMode.MEAN:

@myron considering the tight deadline for the next release, let's use string instead of enum if it can fix the issue and move on.

monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
monai/networks/nets/milmodel.py Show resolved Hide resolved
monai/networks/nets/milmodel.py Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
monai/networks/nets/milmodel.py Outdated Show resolved Hide resolved
Signed-off-by: myron <amyronenko@nvidia.com>
@bhashemian
Copy link
Member

bhashemian commented Nov 14, 2021

@myron it looks good to me. Could you please take a look at the checks that are failing?

@bhashemian
Copy link
Member

@Nic-Ma @wyli do you have any other commnet?

@myron
Copy link
Collaborator Author

myron commented Nov 15, 2021

@myron it looks good to me. Could you please take a look at the checks that are failing?

I don't know why it's failing. There is only 1 failed check, that seems to be unrelated to my PR

2021-11-14 16:01:06,684 - ERROR - check_hash failed 938869fb71b05f5ba345ab36dfbbc4388f53a550.
clang-format: 1.86MB [00:00, 2.41MB/s]                           
Download /__w/MONAI/MONAI/.clang-format-bin/clang-format failed: sha1 check of downloaded file failed: URL=https://oss-clang-format.s3.us-east-2.amazonaws.com/linux64/clang-format-linux64, filepath=/__w/MONAI/MONAI/.clang-format-bin/clang-format, expected sha1=9073602de1c4e1748f2feea5a0782417b20e3043.
Please remove /__w/MONAI/MONAI/.clang-format-bin/clang-format and retry.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 15, 2021

@myron it looks good to me. Could you please take a look at the checks that are failing?

I don't know why it's failing. There is only 1 failed check, that seems to be unrelated to my PR

2021-11-14 16:01:06,684 - ERROR - check_hash failed 938869fb71b05f5ba345ab36dfbbc4388f53a550.
clang-format: 1.86MB [00:00, 2.41MB/s]                           
Download /__w/MONAI/MONAI/.clang-format-bin/clang-format failed: sha1 check of downloaded file failed: URL=https://oss-clang-format.s3.us-east-2.amazonaws.com/linux64/clang-format-linux64, filepath=/__w/MONAI/MONAI/.clang-format-bin/clang-format, expected sha1=9073602de1c4e1748f2feea5a0782417b20e3043.
Please remove /__w/MONAI/MONAI/.clang-format-bin/clang-format and retry.

Hi @myron ,

I already submitted PR to fix it.

Thanks.

@bhashemian bhashemian enabled auto-merge (squash) November 15, 2021 15:35
Copy link
Contributor

@yiheng-wang-nv yiheng-wang-nv left a comment

Choose a reason for hiding this comment

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

Looks good on me.

@bhashemian
Copy link
Member

@Nic-Ma @wyli could you please run Blossom CI here?

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 16, 2021

/build

Copy link
Contributor

@Nic-Ma Nic-Ma 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 great feature.
Looks good to me, I will slightly adjust the doc related things later.

Thanks.

@bhashemian bhashemian merged commit 45d4a61 into Project-MONAI:dev Nov 16, 2021
@myron myron deleted the milmodel branch January 14, 2022 07:54
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.

MIL: add MILModel component
5 participants