Skip to content

rename n_classes to num_classes #2842

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

Merged
merged 8 commits into from
Sep 6, 2021
Merged

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Aug 25, 2021

Fixes #2841

Description

unify n_classes and num_classes across model arguments

Status

Ready/Work in progress/Hold

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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Borda Borda force-pushed the refactor/n_classes branch from e04f434 to 62ad273 Compare August 25, 2021 13:12
@rijobro
Copy link
Contributor

rijobro commented Aug 25, 2021

This is obviously a breaking change, but definitely a useful one.

Similarly, we use dimensions in a few places (e.g., UNet), but use spatial_dims in the majority of other cases.

We also have a couple of different ways of specifying image size if that's necessary, but it's probably more effort than it's worth to correct that.

@wyli
Copy link
Contributor

wyli commented Aug 25, 2021

this PR is probably needed, it is also a good opportunity to test and roll out the deprecated_arg utility before v1.0

def deprecated_arg(
name,
since: Optional[str] = None,
removed: Optional[str] = None,
msg_suffix: str = "",
version_val: str = __version__,
):
"""
Marks a particular named argument of a callable as deprecated. The same conditions for `since` and `removed` as
described in the `deprecated` decorator.
n_class user could get a specific error message @Nic-Ma

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.
Please note to execute the integration tests and update related tutorials.

Thanks.

@Borda
Copy link
Contributor Author

Borda commented Aug 25, 2021

Please note to execute the integration

is I am new in this repository, due to GH policy I need to have approved each CI run by maintainers

update related tutorials

what steps do you suggest, as this is a breaking change, shall this be merged first and update tutorials or do it in parallel?

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Aug 25, 2021

/integration-test

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Aug 25, 2021

I already triggered the integration tests by command, CC @wyli .
Please try to update related tutorials or examples when this PR merged.

Thanks.

@Borda
Copy link
Contributor Author

Borda commented Aug 25, 2021

Similarly, we use dimensions in a few places (e.g., UNet), but use spatial_dims in the majority of other cases.

@rijobro I would address in it separate PR :]

Please try to update related tutorials or examples when this PR merged.

@wyli prepared PR here: Project-MONAI/tutorials#322

this PR is probably needed, it is also a good opportunity to test and roll out the deprecated_arg utility before v1.0

@Nic-Ma I have been using this zero dependence feature: https://pypi.org/project/pyDeprecate/

@wyli
Copy link
Contributor

wyli commented Aug 25, 2021

this will break the current pretrained model loading... (the following works for the dev branch)

import os
from monai.apps.mmars import MODEL_DESC
from monai.apps.mmars import load_from_mmar

# download the MMAR from NGC to local "./" folder
for x in MODEL_DESC:
    pretrained_model = load_from_mmar(item=x, mmar_dir="./", map_location="cpu")

mainly for the breaking changes in:

  • monai/networks/nets/torchvision_fc.py
  • monai/networks/nets/resnet.py

could you please help make the above two files backward compatible for now?

@Borda
Copy link
Contributor Author

Borda commented Aug 25, 2021

could you please help make the above two files backward compatible for now?

@wyli Are you fine with using https://pypi.org/project/pyDeprecate

@wyli
Copy link
Contributor

wyli commented Aug 25, 2021

could you please help make the above two files backward compatible for now?

@wyli Are you fine with using https://pypi.org/project/pyDeprecate

Monai aims at keeping the set of dependencies small for its core modules https://github.com/Project-MONAI/MONAI/blob/dev/requirements.txt we should consider pyDeprecate if it could be an optional dependency instead of a mandatory requirementent. cc @ericspod

@ericspod
Copy link
Member

The deprecated annotations won't be optional so I don't think we can do an optional dependency for pyDeprecate. The idea was to define our own decorator that was just enough for our purposes without being a huge amount of code. pyDeprecate is providing a similar facility but having minimal dependencies is worth the reduplication of code in this instance.

@Borda Borda force-pushed the refactor/n_classes branch 2 times, most recently from bebf604 to cb8d93a Compare August 26, 2021 09:57
@Borda
Copy link
Contributor Author

Borda commented Aug 26, 2021

@ericspod I have added back-compatibility path, mind check if I use the deprecated_arg correctly?

@Borda
Copy link
Contributor Author

Borda commented Aug 26, 2021

/integration-test

@Borda
Copy link
Contributor Author

Borda commented Aug 26, 2021

I already triggered the integration tests by command

@Nic-Ma it seems that /integration-test does not work for me :] does not start anything
image

@wyli
Copy link
Contributor

wyli commented Aug 26, 2021

The /integration-test is done here https://github.com/Project-MONAI/MONAI/actions/runs/1169956359

I think there are some issue with the utility function. How about we remove the decorator deprecated_arg for now and merge this PR, then have a follow-up on fixing deprecated_arg?

@Borda
Copy link
Contributor Author

Borda commented Aug 26, 2021

just a noob question why Flake8 CI is complaining about Isort formating?
also seems that the script does not work locally:

~/Workingspace/MONAI$ ./runtests.sh --autofix
PYTHONPATH: /home/jirka/Workingspace/MONAI:
python: /usr/bin/python
./runtests.sh: line 105: 3065342 Segmentation fault      (core dumped) ${cmdPrefix}${PY_EXE} -c "import monai"

so I just just isort . which could be easily fused with #2843

@Borda Borda force-pushed the refactor/n_classes branch from f951fd4 to 6f97f4b Compare August 26, 2021 12:29
@Borda
Copy link
Contributor Author

Borda commented Aug 26, 2021

/build

@wyli
Copy link
Contributor

wyli commented Aug 26, 2021

looks like the pre-commit ci doesn't read the black config

MONAI/pyproject.toml

Lines 13 to 17 in ce139f6

exclude = '''
(
/(
# exclude a few common directories in the root of the project
\.eggs
, I'll disable those for now

@wyli wyli force-pushed the refactor/n_classes branch 4 times, most recently from da77414 to fd7d40f Compare August 27, 2021 15:56
@madil90
Copy link
Contributor

madil90 commented Aug 30, 2021

/build

@Borda Borda force-pushed the refactor/n_classes branch from fd7d40f to f7bf296 Compare August 30, 2021 08:35
@Borda
Copy link
Contributor Author

Borda commented Aug 30, 2021

any suggestion on this typing issue:

monai/networks/nets/resnet.py:322:5: error: Returning Any from function declared to return "ResNet"

while I have not touched any return types... could be related to using the deprecated_arg wrapper?

@wyli
Copy link
Contributor

wyli commented Aug 30, 2021

any suggestion on this typing issue:

monai/networks/nets/resnet.py:322:5: error: Returning Any from function declared to return "ResNet"

while I have not touched any return types... could be related to using the deprecated_arg wrapper?

you're right, I think the wrapper needs some enhancements, do you have time to look into deprecated_arg or please just put a "type ignore" to skip the check

@ericspod
Copy link
Member

ericspod commented Sep 1, 2021

The decorator might have an issue with it's type or an issue with constructors but it's being used correct I believe.

Signed-off-by: Jirka <[email protected]>
Signed-off-by: Jirka <[email protected]>
Signed-off-by: Jirka <[email protected]>
bhashemian and others added 3 commits September 6, 2021 17:56
* Implement SplitOnGrid

Signed-off-by: Behrooz <[email protected]>

* Implement dictionary-based SplitOnGrid

Signed-off-by: Behrooz <[email protected]>

* Update inits

Signed-off-by: Behrooz <[email protected]>

* Update docs

Signed-off-by: Behrooz <[email protected]>

* Change imports

Signed-off-by: Behrooz <[email protected]>

* Update input logic in SplitOnGrid)

Signed-off-by: Behrooz <[email protected]>

* Add unittests for SplitOnGrid and SplitOnGridDict

Signed-off-by: Behrooz <[email protected]>

* Sort import

Signed-off-by: Behrooz <[email protected]>

* Remove imports

Signed-off-by: Behrooz <[email protected]>

* Address comments

Signed-off-by: Behrooz <[email protected]>

* Remove optional

Signed-off-by: Behrooz <[email protected]>

* Address thread safety issues

Signed-off-by: Behrooz <[email protected]>
…roject-MONAI#2900)

* [DLMED] fix type issue

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix test

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] simplify the change

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix flake8

Signed-off-by: Nic Ma <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the refactor/n_classes branch from d8477a6 to 76f490d Compare September 6, 2021 16:56
@wyli wyli enabled auto-merge (squash) September 6, 2021 17:01
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli disabled auto-merge September 6, 2021 17:28
@wyli wyli enabled auto-merge (squash) September 6, 2021 17:29
@wyli wyli merged commit 1ecf5b6 into Project-MONAI:dev Sep 6, 2021
@Borda Borda deleted the refactor/n_classes branch September 6, 2021 19:23
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.

Unify general classes argument - n_classes vs num_classes
7 participants