Skip to content

Add dtype to ScaleIntensityRange transforms#3235

Merged
wyli merged 18 commits intoProject-MONAI:devfrom
Nic-Ma:fix-csviterabledataset
Nov 2, 2021
Merged

Add dtype to ScaleIntensityRange transforms#3235
wyli merged 18 commits intoProject-MONAI:devfrom
Nic-Ma:fix-csviterabledataset

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Nov 1, 2021

Fixes #3234 .

Description

This PR added dtype arg to ScaleIntensityRange transforms, same as other intensity scale transforms which may change data type during computation, especially for int or long input.

Status

Ready

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.

Nic-Ma and others added 17 commits February 1, 2021 19:15
@Nic-Ma
Merge pull request #19 from Project-MONAI/master
42a45e0 
merge master
@Nic-Ma
Merge pull request #32 from Project-MONAI/master
cd16a13 
merge master
@Nic-Ma
Merge pull request #180 from Project-MONAI/dev
6f87afd 
merge master
@Nic-Ma
Merge pull request #214 from Project-MONAI/dev
f398298 
merge master
@Nic-Ma
Merge pull request #267 from Project-MONAI/dev
9a74e4f 
merge master
@Nic-Ma
[DLMED] fix 2 CSVIterableDataset issues
1e2b8db 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
[DLMED] fix length issue
ea64232 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
[DLMED] add transform
7e3747d 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Merge branch 'dev' into fix-csviterabledataset
d713376
@Nic-Ma
[DLMED] simplify the logic
b5bc535 
Signed-off-by: Nic Ma <[email protected]>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
790c266 
for more information, see https://pre-commit.ci
@Nic-Ma
Merge branch 'dev' into fix-csviterabledataset
8867934
@Nic-Ma
Merge branch 'dev' into fix-csviterabledataset
e8e38df
@Nic-Ma
[DLMED] remove get_items
6eceb33 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
[DLMED] update according to comments
3d394fc 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
[DLMED] add dtype
f4c8daa 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Merge branch 'dev' into fix-csviterabledataset
9700239
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 1, 2021

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 1, 2021

/build

@Nic-Ma Nic-Ma mentioned this pull request Nov 1, 2021
Merged
yiheng-wang-nv
yiheng-wang-nv reviewed Nov 2, 2021
View reviewed changes
@Nic-Ma Nic-Ma requested a review from yiheng-wang-nv November 2, 2021 04:16
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 2, 2021

I also fixed the data in / out type issue in histogram_normalize transform.

Thanks.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 2, 2021

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 2, 2021

/build

@myron
Copy link
Copy Markdown
Collaborator

myron commented Nov 2, 2021

Thanks for this!

Nic-Ma
Nic-Ma commented Nov 2, 2021
View reviewed changes
Nic-Ma
Nic-Ma commented Nov 2, 2021
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 2, 2021

/integration-test

wyli
wyli requested changes Nov 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I feel in many cases now it's good to use the CastToType to indicate type casting, perhaps this needs more discussion? dtype parameter may be useful to control the memory footprint or float precision of the transform computations.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Nov 2, 2021

I feel in many cases now it's good to use the CastToType to indicate type casting, perhaps this needs more discussion?

Hi @wyli ,

I agree with you, we should use CastToType for most cases. Maybe this PR changed many things that confused you.
Actually, here I didn't add dtype to more transforms, just added to ScaleIntensityRange, because data type may be changed during scaling, just followed the same pattern as ScaleIntensity transform.
All the other changes are to add support for dtype=None.
Could you please help take a look again?

Thanks.

wyli
wyli approved these changes Nov 2, 2021
View reviewed changes
Copy link
Copy Markdown
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, we agreed to have this based on the discussions.

@wyli wyli merged commit 223476b into Project-MONAI:dev Nov 2, 2021
@wyli wyli mentioned this pull request Nov 2, 2021
Merged
7 tasks
@Nic-Ma Nic-Ma mentioned this pull request Nov 2, 2021
Merged
7 tasks
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

@yiheng-wang-nv yiheng-wang-nv yiheng-wang-nv approved these changes

@rijobro rijobro Awaiting requested review from rijobro

@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

Development

Successfully merging this pull request may close these issues.

Add dtype arg to ScaleIntensityRange transform for output

4 participants

@Nic-Ma @myron @wyli @yiheng-wang-nv