Skip to content

Conversation

@Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Jun 23, 2021

Fixes #2420 .

Description

According to user's feedback, this PR added the image_only option for several spatial transforms, same as the LoadImage IO transform.

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
Copy link
Contributor Author

Nic-Ma commented Jun 23, 2021

/black

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 2420-add-image-only branch from 12d3200 to 2f2c4f2 Compare June 23, 2021 02:53
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jun 23, 2021

/black

@Nic-Ma Nic-Ma requested review from ericspod, rijobro and wyli June 23, 2021 02:54
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma enabled auto-merge (squash) June 23, 2021 03:21
@Nic-Ma Nic-Ma merged commit a0afa60 into Project-MONAI:dev Jun 23, 2021
@wyli
Copy link
Contributor

wyli commented Jun 23, 2021

I agree with the comment here #2426 (comment) probably we shouldn't do this for spacing and orientation.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jun 23, 2021

Hi @wyli ,

Thanks for your comments.
I think your point is that: even only returning the image, the Spacing and Orientation transforms still need img and affine as input args, so they can't be composed in a chain, right?
I added image_only to them because I think the affine arg of Spacing and Orientation is optional, so if no need to provide affine matrix in some case(use default np.eye()), this image_only can be helpful, and it's non-breaking.
What do you think?

Thanks.

@wyli
Copy link
Contributor

wyli commented Jun 23, 2021

I think it's incorrect for Spacing and Orientation because these have to be in the context of affine/coordinate systems

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jun 23, 2021

I think it's incorrect for Spacing and Orientation because these have to be in the context of affine/coordinate systems

OK, I thought Spacing and Orientation can work without input affine because it's optional arg.
Let me remove the image_only arg in #2423.

Thanks.

@Nic-Ma Nic-Ma deleted the 2420-add-image-only branch July 2, 2021 23:36
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.

Transforms: Spacing output not compatible with CenterScaleCrop

3 participants