Skip to content

Conversation

@finalelement
Copy link
Collaborator

@finalelement finalelement commented May 13, 2022

Signed-off-by: vnath vnath@nvidia.com

Fixes # .

Description

This PR will be for working upon the transforms of NuClick for Pathology annotation AI

Status

Work in progress

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.

Signed-off-by: vnath <vnath@nvidia.com>
@finalelement finalelement marked this pull request as draft May 13, 2022 01:34
Signed-off-by: vnath <vnath@nvidia.com>
@bhashemian
Copy link
Member

Hi @wyli @Nic-Ma @ericspod @rijobro,

There are four transforms here related to NuClick in MONAI Label that needs your help to identify the best place for them in MONAI Core or outside (MONAI Label, tutorials, etc.).
Based on our previous discussions, monai/app might not be the best place for hosting all of them. I was wondering if you could help to evaluate them to see:

  • if there is any exisiting MONAI component that can do the task,
  • if the component might be useful for other use cases so we can add it to MONAI core.
  • wether the component belongs to other repo like MONAI label, or just part of the script in tutorial.

Many thanks

  1. class FlattenLabeld(MapTransform):
  2. class ExtractPatchd(MapTransform):
  3. class SplitLabeld(Transform):
  4. class AddPointGuidanceSignald(RandomizableTransform):

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 19, 2022

Hi @drbeh ,

Thanks for raising it.
We already have some similar transforms in MONAI core:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L960
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L286
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L773
I think you are expert on Pathology, could you please help update the existing transforms or add new transforms? Or keep some task-specific transforms in apps/nuclick?
Then please update the PR for review.

Thanks in advance.

@bhashemian
Copy link
Member

@finalelement, could you please take a look at these components that Nic has shared to see if any of them can replace the transforms in nuclick? thanks

class SplitDim(Transform):

class LabelToMask(Transform):

class AddExtremePointsChannel(Randomizable, Transform):

@finalelement
Copy link
Collaborator Author

@drbeh Thank you for the highlight. @Nic-Ma Thanks for sharing the transforms. Ill check them and let you guys know if I see any synergies.

@finalelement
Copy link
Collaborator Author

finalelement commented May 19, 2022

@Nic-Ma @drbeh @wyli

I looked into the shared transforms, that were shared by Nic:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L960
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L286
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/utility/array.py#L773)

SplitDim & AddExtremePointsChannel are not directly applicable here, with the transforms that are being proposed for nuclick. The one transform that I see that there are synergies is LabeltoMask which it shares in this proposed one SplitLabeld. Even then LabeltoMask would require changes as SplitLabeld also returns the remaining labels as 'others' which is used later on in the NuClick transforms.

We can work towards integrating this functionality in another PR and then eventually replace the one in NuClick app transforms for this functionality.

@finalelement
Copy link
Collaborator Author

Also, the first check of 'CodeQL / Analyze (cpp) (pull_request)' is failing because we don't have the library cv2 in our requirements-dev.txt.

I was wondering if it is straight forward to include it in the requirements-dev.txt or some process is required? @wyli @Nic-Ma

Signed-off-by: vnath <vnath@nvidia.com>
Signed-off-by: vnath <vnath@nvidia.com>
…dev requirements

Signed-off-by: vnath <vnath@nvidia.com>
Signed-off-by: vnath <vnath@nvidia.com>
Signed-off-by: vnath <vnath@nvidia.com>
@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 20, 2022

Hi @finalelement ,

I think you can add opencv as an optional import, just search pandas in the codebase, you will get where to change.
Refer to: #4259 (comment). @wyli Do you have any other concerns about OpenCV lib?

Thanks.

Signed-off-by: vnath <vnath@nvidia.com>
@finalelement
Copy link
Collaborator Author

@Nic-Ma Thanks for the share, that is very helpful. I can make the changes at all the suggested files for adding python-opencv.

Could you please also offer suggestions on this test (build / flake8-py3 (pull_request) for the PR, the autofix is not getting it and I am not sure what fix in terms of formatting to resolve this test.

@finalelement
Copy link
Collaborator Author

finalelement commented May 20, 2022

Also @Nic-Ma @wyli. I've figured out most of the tests that are needed for this PR. This polishing is left from my side:

  • Init function, tie data types to all variables
  • Polish the doc strings
  • HTML & Doc building

In the meanwhile, feel free to provide any high level errors that I might have made. I'll take care of the above things tomorrow and try to have it ready for review

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 20, 2022

The flake8 error message is : /home/runner/work/MONAI/MONAI/tests/test_nuclick_transforms.py:103:1: E302 expected 2 blank lines, found 1

Thanks.

@finalelement
Copy link
Collaborator Author

finalelement commented May 20, 2022

@Nic-Ma Thanks the above is resolved now, I had been looking at the incorrect file, which is why it didn't make sense.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 20, 2022

/black

Signed-off-by: vnath <vnath@nvidia.com>
Signed-off-by: vnath <vnath@nvidia.com>
Signed-off-by: vnath <vnath@nvidia.com>
@bhashemian bhashemian disabled auto-merge May 26, 2022 16:12
@bhashemian
Copy link
Member

I have few comments that I'm going to add.

@wyli
Copy link
Contributor

wyli commented May 26, 2022

I have few comments that I'm going to add.

thanks, could you please send a pull request to this branch directly? @drbeh

@bhashemian
Copy link
Member

bhashemian commented May 26, 2022

I have few comments that I'm going to add.

thanks, could you please send a pull request to this branch directly? @drbeh

pull request?! why is that? It's already a pull request. Sorry, I didn't get your point @wyli.

@wyli
Copy link
Contributor

wyli commented May 26, 2022

I have few comments that I'm going to add.

thanks, could you please send a pull request to this branch directly? @drbeh

@wyli, pull request?! why is that?

Hi @drbeh,

please follow our code of conduct for efficient communication. https://github.com/Project-MONAI/MONAI/blob/dev/CODE_OF_CONDUCT.md

@bhashemian
Copy link
Member

@finalelement, thank you very much for this PR and implementing several components. To make sure that this code is matching the style of MONAI, I have left some comments in the code. Could you please review them and update the PR? Please let me know if you have any question.

@finalelement
Copy link
Collaborator Author

@drbeh I've addressed most of the comments, please see.

@bhashemian
Copy link
Member

@drbeh I've addressed most of the comments, please see.

Thank you very much @finalelement!

@bhashemian
Copy link
Member

Hi @wyli, if it doesn't have to have the same code quality as monai core, please feel free to merge. Thanks

@finalelement
Copy link
Collaborator Author

@Nic-Ma Thank you for the suggestions, changes made, commit en route.

finalelement and others added 4 commits May 27, 2022 15:34
@finalelement
Copy link
Collaborator Author

@Nic-Ma all changes made

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.

Looks good to me just a missing doc-string and unit test.

Thanks for the quick update.

…-string

Signed-off-by: vnath <vnath@nvidia.com>
@finalelement
Copy link
Collaborator Author

@Nic-Ma

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 31, 2022

/black

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 31, 2022

/build

@Nic-Ma Nic-Ma enabled auto-merge (squash) May 31, 2022 16:21
@Nic-Ma Nic-Ma merged commit 554de62 into Project-MONAI:dev May 31, 2022
@finalelement finalelement self-assigned this May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 💯 Complete

Development

Successfully merging this pull request may close these issues.

7 participants