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.

Sign up for GitHub

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

Jump to bottom

Improve Compose encapsulation #6224

Merged
merged 33 commits into from
Mar 30, 2023
Merged

Improve Compose encapsulation #6224

merged 33 commits into from
Mar 30, 2023

Conversation

atbenmurray
Copy link
Contributor

Fixes #6223 .

Description

A few sentences describing the changes proposed in this pull request.

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.

@wyli
Copy link
Contributor

wyli commented Mar 22, 2023 •
edited
Loading

/black

@atbenmurray
Copy link
Contributor Author

@ericspod, @Nic-Ma, @rijobro, @wyli
I'm writing up the test extensions for this, but please take a look at the code changes when you have a moment. I'm essentially trying to eliminate the need for dataset.py to have to be updated by the lazy resampling changes coming up in #5860.

monai-bot and others added 3 commits March 22, 2023 14:06
@atbenmurray
DCO Remediation Commit for Ben Murray <[email protected]>
10edabd 
I, Ben Murray <[email protected]>, hereby add my Signed-off-by to this commit: 1759f96

Signed-off-by: Ben Murray <[email protected]>q:
@atbenmurray
DCO Remediation Commit for Ben Murray <[email protected]>
5a8ccaf 
I, Ben Murray <[email protected]>, hereby add my Signed-off-by to this commit: 1759f96

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Merge branch 'compose_refactor' of github.com:project-monai/monai int…
918cbf6 
…o compose_refactor
@atbenmurray
DCO Remediation Commit for Ben Murray <[email protected]>
c3bd8c0 
I, Ben Murray <[email protected]>, hereby add my Signed-off-by to this commit: 10edabd

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Fixes to make test_cachedataset, test_persistentdataset and test_cach…
0c0baa4 
…edataset_persistent_workers pass

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

Taking the opportunity boost the docstrings a little. It looks as if logging only does anything when the pipeline raises an exception of some kind, so the comment for that parameter should be updated, yes?

@wyli
Copy link
Contributor

wyli commented Mar 22, 2023

I think the refactoring makes sense, let me trigger some integration tests about multiprocessing

wyli
wyli approved these changes Mar 23, 2023
View reviewed changes
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.

the integration tests work fine, could you please revise the docstrings, and in my opinion we can merge this. more tests with Compose.execute would be great as well.

Nic-Ma
Nic-Ma approved these changes Mar 23, 2023
View reviewed changes
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.

The change logic looks good to me, put minor comments inline.
Please complete the doc-strings and double-confirm the threading arg in the self.transform() function of every dataset class.

Thanks.

monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/data/dataset.py Outdated Show resolved Hide resolved
@atbenmurray
Copy link
Contributor Author

@wyli @Nic-Ma what is the issue with the failure? Not sure what is causing it

@wyli
Copy link
Contributor

wyli commented Mar 24, 2023

@wyli @Nic-Ma what is the issue with the failure? Not sure what is causing it

those tests are optional and deprecating... please ignore

@wyli wyli mentioned this pull request Mar 28, 2023
Closed
7 tasks
@atbenmurray
Copy link
Contributor Author

atbenmurray commented Mar 28, 2023 •
edited
Loading

If everyone is happy review-wise, I'll sort out the remaining reds in anticipation of a merge
@ericspod suggested that I make Compose.execute a function rather than having it as a class method, so I'll do that first before we review for merge.

@wyli
Copy link
Contributor

wyli commented Mar 28, 2023

If everyone is happy review-wise, I'll sort out the remaining reds in anticipation of a merge

please update the docstring of the datasets to clarify the implicit execution of pending operations during caching. otherwise this looks good to me.

@atbenmurray
Moving Compose.execute to execute_compose as per @ericspod's request.…
d939395 
… Documenting lazy resampling functionality and updating dataset docs to refer to it as per @wyli's request

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Test fix: missed Compose.execute to execute_compose changes
4ecf2c3 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
DCO Remediation Commit for Ben Murray <[email protected]>
948444f 
I, Ben Murray <[email protected]>, hereby add my Signed-off-by to this commit: 6bdedac

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Bug fix for SomeOff; generate list of transforms in execution order
9097e07 
through list comprehension

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Documentation for Compose.get_index_of_first
d701642 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Slight documentation reformatting for Compose.get_index_of_first
77e465d 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Updated docstrings for execute_compose. Renamed input_ to data for
5605e71 
execute_compose.

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Fixing errors reported by flake8-py3 (mypy) output
c06e014 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
Had to go back to lazy_evaluation default of None for now but this is a
191506d 
critical design flaw that needs fixing

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
execute_compose type ignore as it can't be fixed without polluting more
ec71402 
code with an incorrect type

Signed-off-by: Ben Murray <[email protected]>
@atbenmurray
type: ignore suppression as this is being addressed separately
85b18de 
Signed-off-by: Ben Murray <[email protected]>
@atbenmurray atbenmurray marked this pull request as ready for review March 30, 2023 09:35
@wyli
Copy link
Contributor

wyli commented Mar 30, 2023

/build

@wyli wyli enabled auto-merge (squash) March 30, 2023 13:30
@wyli wyli merged commit e4d48f0 into dev Mar 30, 2023
@wyli wyli deleted the compose_refactor branch March 30, 2023 13:59
a-parida12 pushed a commit to a-parida12/MONAI that referenced this pull request Apr 3, 2023
@atbenmurray @a-parida12
Improve Compose encapsulation (Project-MONAI#6224)
5ea2aa8 
Fixes Project-MONAI#6223 .

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] 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: Ben Murray <[email protected]>
Signed-off-by: a-parida12 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wyli wyli wyli approved these changes

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

@KumoLiu KumoLiu KumoLiu left review comments

@ericspod ericspod Awaiting requested review from ericspod

@rijobro rijobro Awaiting requested review from rijobro

Assignees
No one assigned
Labels
None yet
Projects
No open projects
MONAI v1.2
Status: Done
Milestone
Lazy resampling (+ cache-based datase...
6 participants
@atbenmurray @wyli @ericspod @Nic-Ma @KumoLiu @monai-bot