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

9 intensity normalisation transform #25

Merged
merged 4 commits into from
Jan 22, 2020
Merged

9 intensity normalisation transform #25

merged 4 commits into from
Jan 22, 2020

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Jan 14, 2020 •
edited
Loading

Hi All,

This PR implemented the intensity normalization transform for task #9
Designed according to our latest discussion:
1. input data is dict format with keys for fields.
2. only based on PyTorch and data shape is channel_last.

Could you please help review it when you are available?
Thanks in advance.

I verified this PR locally with the below program:

# test 1 key with auto subtrahend and divisor
normalizer = IntensityNormalizer(apply_keys=('image',))
img = np.arange(27).reshape(3, 3, 3).astype(np.float32)
label = np.arange(27).reshape(3, 3, 3).astype(np.float32)
data = {'image': img, 'label': label}
print(data)
print(normalizer(data=data))

# test 2 keys with specified subtrahend and divisor
normalizer = IntensityNormalizer(
    apply_keys=('image', 'label'),
    subtrahend=np.ones((3), dtype=np.float32),
    divisor=np.ones((3), dtype=np.float32)
)
img = np.arange(27).reshape(3, 3, 3).astype(np.float32)
label = np.arange(27).reshape(3, 3, 3).astype(np.float32)
data = {'image': img, 'label': label}
print(data)
print(normalizer(data=data))

@Nic-Ma Nic-Ma mentioned this pull request Jan 14, 2020
Closed
@wyli
Copy link
Contributor

wyli commented Jan 20, 2020

please update based on #9 (comment)

@Nic-Ma
[DLMED] implement intensity normalization transform
41fd036 
design according to our latest discussion:
1. input data is dict format with keys for fields.
2. only based on PyTorch and data shape is channel_last.
@Nic-Ma Nic-Ma force-pushed the 9-intensity-normalisation-transform branch from 8cd6e33 to 41fd036 Compare January 21, 2020 05:02
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2020

Hi @wyli @ericspod @atbenmurray ,

I already update the code and PR description according to our latest discussion.
Could you please help review it again?
Thanks in advance.

wyli and others added 2 commits January 21, 2020 23:16
@wyli @ericspod
@IsaacYangSLA @Nic-Ma
9 part a adding test intensity normalisation transform (#33)
e31ec10 
* Adding script to run unit tests and example test cases (#29)

Adding script to run unit tests and example test cases

* initial unit tests for dice loss (#27)

* initial unit tests for 2d/3d unet
* unit tests update
 - triggering unit tests via github workflow
 - renamed testconvolutions.py to test_convolutions.py
 - test unet test cases as variables for readability

* initial unit tests for 2d/3d unet (#26)

* initial unit tests for 2d/3d unet
* unit tests update
 - triggering unit tests via github workflow
 - renamed testconvolutions.py to test_convolutions.py
 - test unet test cases as variables for readability

* 14 code examples of monai input data pipeline (#24)

* fixes cardiac example

* update example cardiac segmentation

* Create .gitlab-ci.yml (#30)

an initial step towards #19

* tests intensity normalizer

- revised to support both `[key]` and `key` as an input for apply_keys
- added `NumpyImageTestCase2D` and `TorchImageTestCase2D`

* style updates and new test cases:

- adding copyright notice
- validate user input before setting class member
- one line space after copyright
- testing multiple keys input data

Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Co-authored-by: Isaac Yang <isaacy@nvidia.com>
@wyli
Merge branch 'master' into 9-intensity-normalisation-transform
70e675d
@wyli
Copy link
Contributor

wyli commented Jan 21, 2020

@ericspod could you review this PR? mainly added a minimal transform interface, and updated ImageTestCase to have NumpyImageTestCase2D and TorchImageTestCase2D .

@wyli wyli removed their request for review January 21, 2020 15:35
@wyli wyli mentioned this pull request Jan 21, 2020
Merged
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 22, 2020

Hi @ericspod @atbenmurray @wyli ,

I updated this PR to remove dict-key processing and be compatible with Eric's other transforms.
Could you please help review again?
Thanks in advance.

@Nic-Ma Nic-Ma force-pushed the 9-intensity-normalisation-transform branch from 2d9c037 to a2a4e23 Compare January 22, 2020 01:18
ericspod
ericspod approved these changes Jan 22, 2020
View reviewed changes
tests/test_intensity_normalizer.py
self.assertTrue(np.allclose(normalised, expected))


if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

Generally you don't need a main section in the test case scripts, you can run tests from the root directory with

python -m unittest test/test_intensity_normalizer.py

It doesn't hurt to be here though.

Copy link
Contributor

@wyli wyli Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! @atbenmurray could you please update the contribution guidelines about running all unit tests and single unit test . PR for new features should include new unit tests and inherit test case base classes.

@ericspod
Copy link
Member

Beyond the issue with the CI pipeline not completing the code is good to go.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 22, 2020

Beyond the issue with the CI pipeline not completing the code is good to go.

Hi @ericspod ,

Thanks for your quick review!
I think maybe gitlab-staging server is unavailable now.
The gitlab-master server passed tests.
Of course, I am OK if you think it's better to wait for the stage server recovery.
Thanks.

@ericspod
Copy link
Member

I would just go ahead with the merge.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 22, 2020

I would just go ahead with the merge.

Cool, thanks!
Could you please help merge it directly?

@ericspod ericspod merged commit 1390701 into master Jan 22, 2020
@wyli wyli deleted the 9-intensity-normalisation-transform branch April 6, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wyli wyli wyli left review comments

@ericspod ericspod ericspod approved these changes

@atbenmurray atbenmurray Awaiting requested review from atbenmurray

@yanchengnv yanchengnv Awaiting requested review from yanchengnv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nic-Ma @wyli @ericspod