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

7047 simplify resnet pretrained #7095

Merged
merged 39 commits into from
Oct 18, 2023

Conversation

vgrau98
Copy link
Contributor

@vgrau98 vgrau98 commented Oct 6, 2023

Fixes #7047

Description

Resnet did not support True value (not implemented ) for its pretrained flag.
2 implemented behavior:

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.

Fixes: Project-MONAI#7047

Original behaviour did not support True pretrained flag.

Signed-off-by: vgrau98 <[email protected]>
Signed-off-by: vgrau98 <[email protected]>
@vgrau98
Copy link
Contributor Author

vgrau98 commented Oct 6, 2023

Not sure about good practices in added test

@vgrau98
Copy link
Contributor Author

vgrau98 commented Oct 6, 2023

Documentation should be updated too

@vgrau98 vgrau98 force-pushed the 7047-simplify-resnet-pretrained branch from e6a29e5 to 8b75095 Compare October 6, 2023 16:26
@vgrau98
Copy link
Contributor Author

vgrau98 commented Oct 8, 2023

test_resnet gives errors, all the tests i wrote don't seem to be unitary, there's an integration part with the huggingface api. I'll try to fix this quickly by creating integration tests in the dedicated test_integration_resnet.py file. Maybe the test environment is not the same between unit and integration tests ?

@wyli
Copy link
Contributor

wyli commented Oct 9, 2023

/black

test_resnet gives errors, all the tests i wrote don't seem to be unitary, there's an integration part with the huggingface api. I'll try to fix this quickly by creating integration tests in the dedicated test_integration_resnet.py file. Maybe the test environment is not the same between unit and integration tests ?

thanks, please follow this guide for adding new dep https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md#adding-new-optional-dependencies

I think the current unit test looks great, we just need to somehow pin down a specific commit hash of the model version, then I don't see much risk of regression.

@vgrau98
Copy link
Contributor Author

vgrau98 commented Oct 9, 2023

test_resnet gives errors, all the tests i wrote don't seem to be unitary, there's an integration part with the huggingface api. I'll try to fix this quickly by creating integration tests in the dedicated test_integration_resnet.py file. Maybe the test environment is not the same between unit and integration tests ?

thanks, please follow this guide for adding new dep https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md#adding-new-optional-dependencies

I think the current unit test looks great, we just need to somehow pin down a specific commit hash of the model version, then I don't see much risk of regression.

Yes sorry my bad, didn't read the doc carefully enough, should be fixed now

@vgrau98 vgrau98 marked this pull request as ready for review October 11, 2023 22:40
Copy link
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.

Looks good to me, several minor comments inline.
Could you please also fix the format ci error?
https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md#checking-the-coding-style

monai/networks/utils.py Outdated Show resolved Hide resolved
monai/networks/utils.py Outdated Show resolved Hide resolved
monai/networks/nets/resnet.py Outdated Show resolved Hide resolved
@wyli
Copy link
Contributor

wyli commented Oct 18, 2023

/build

1 similar comment
@wyli
Copy link
Contributor

wyli commented Oct 18, 2023

/build

@wyli wyli enabled auto-merge (squash) October 18, 2023 19:20
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks! trying to merge this pr for more integration tests

Signed-off-by: Wenqi Li <[email protected]>
@wyli
Copy link
Contributor

wyli commented Oct 18, 2023

/build

@wyli wyli merged commit 9f1168f into Project-MONAI:dev Oct 18, 2023
23 checks passed
@vgrau98 vgrau98 deleted the 7047-simplify-resnet-pretrained branch October 18, 2023 21:06
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.

Simplify existing resnet API for pretrained flag
4 participants