Skip to content

2613 saver/loader consistencies#2615

Merged
wyli merged 15 commits intoProject-MONAI:devfrom
wyli:1629-update-itk-dep
Jul 22, 2021
Merged

2613 saver/loader consistencies#2615
wyli merged 15 commits intoProject-MONAI:devfrom
wyli:1629-update-itk-dep

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Jul 16, 2021 •
edited
Loading

fixes #2613
fixes #1629
fixes #1357
fixes #1713 (by upgrading itk)
fixes #1711

Description

  • using the numpy array layout for the get_data of itkreader,pilreader, so that the loader results are consistent
  • itkreader's affine is converted to nibabel's convention to be consistent
  • python -m tests.test_distributed_sampler still gives messages such as UserWarning: semaphore_tracker: There appear to be 1 leaked semaphores to clean up at shutdown because of itk 5.2.0 semaphore leaks addressed by workaround ac7944e

Status

Ready

Types of changes

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

@wyli wyli force-pushed the 1629-update-itk-dep branch 3 times, most recently from 4a86ac6 to 24c71e1 Compare July 16, 2021 09:01
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jul 16, 2021 •
edited
Loading

/integration-test

/black

@wyli wyli force-pushed the 1629-update-itk-dep branch from 24c71e1 to 4a86ac6 Compare July 16, 2021 09:33
@wyli wyli marked this pull request as ready for review July 16, 2021 14:54
@wyli wyli changed the title [wip] 2613 saver/loader consistencies 2613 saver/loader consistencies Jul 16, 2021
ericspod
ericspod reviewed Jul 16, 2021
View reviewed changes
@madil90
Copy link
Copy Markdown
Contributor

madil90 commented Jul 19, 2021

/build

@wyli wyli requested a review from ericspod July 19, 2021 22:53
@wyli wyli force-pushed the 1629-update-itk-dep branch from 04b2876 to ce62b54 Compare July 21, 2021 11:47
wyli added 13 commits July 21, 2021 09:18
@wyli
fixes itk reader
9b081d8 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
adds consistency tests
704aa1b 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
multichannel RW support
7ba0996 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
fixes integration
a726b9d 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
update
302fe9d 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
revert answer
7306678 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
fixes integration
ddacbe4 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
revert unused changes
832b0d6 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
formatting
cac108b 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
resume tests
3232dba 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
fixes semaphore leak
5d733ae 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
update spatial rank get
60b063d 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
adds integration tests
7835727 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the 1629-update-itk-dep branch from ce62b54 to 7835727 Compare July 21, 2021 13:23
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jul 21, 2021

I run a few tutorials/demo with this PR and all work fine. Additional integration tests show that loadimaged with the 'itkreader' or 'nibabelreader' option gives the same results.

MONAI/tests/test_integration_segmentation_3d.py

Lines 254 to 257 in 7835727

if idx == 1:
_readers = ("itkreader", "itkreader")
elif idx == 2:
_readers = ("itkreader", "nibabelreader")

I think this gives us some confidence in the code. Could you please review @ericspod @rijobro @Nic-Ma? The current itkreader in the dev branch is broken #1711, which means we don't have any basic working loader for DICOM.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jul 21, 2021 •
edited
Loading

Hi @wyli ,

Thanks for your quick update and test.
I just came back from a short vacation last night and still checking the ITK APIs related to the data loading and thinking about how to develop the saving feature for other formats, will review this PR ASAP.

Thanks.

Nic-Ma
Nic-Ma approved these changes Jul 21, 2021
View reviewed changes
Copy link
Copy Markdown
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 except for several minor comments.

Thanks.

@wyli wyli force-pushed the 1629-update-itk-dep branch from 167a933 to 116eaa5 Compare July 21, 2021 18:52
@wyli wyli enabled auto-merge (squash) July 21, 2021 19:16
@wyli wyli disabled auto-merge July 21, 2021 21:50
@wyli wyli enabled auto-merge (squash) July 22, 2021 00:31
@wyli wyli merged commit 027947b into Project-MONAI:dev Jul 22, 2021
@wyli wyli deleted the 1629-update-itk-dep branch July 22, 2021 09:46
@wyli wyli mentioned this pull request Jul 22, 2021
Merged
1 task
Nic-Ma
Nic-Ma reviewed Jul 26, 2021
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@rijobro rijobro Awaiting requested review from rijobro

@yiheng-wang-nv yiheng-wang-nv Awaiting requested review from yiheng-wang-nv

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@wyli @madil90 @Nic-Ma @ericspod