Skip to content

Invert some keys#2252

Closed
rijobro wants to merge 5 commits into
Project-MONAI:devfrom
rijobro:invert_some_keys2
Closed

Invert some keys#2252
rijobro wants to merge 5 commits into
Project-MONAI:devfrom
rijobro:invert_some_keys2

Conversation

@rijobro

@rijobro rijobro commented May 26, 2021

Copy link
Copy Markdown
Contributor

Invert some but not all transforms.

To skip the inverse of RandAdjustContrastd, for example, this can be done with:

d_fwd = transforms(d)
d_fwd_inv = transforms.inverse_with_omissions(d, "RandAdjustContrastd")

Skipped transforms will be added to a list in the dictionary e.g., image_skipped_inverses.

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.

rijobro added 2 commits May 26, 2021 12:59
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro rijobro requested review from Nic-Ma, ericspod and wyli May 26, 2021 12:12
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro rijobro changed the title Invert some keys2 Invert some keys May 26, 2021
rijobro added 2 commits May 26, 2021 14:03
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@Nic-Ma

Nic-Ma commented May 26, 2021

Copy link
Copy Markdown
Contributor

Hi @rijobro ,

Thanks for the quick PR.
I have several comments here:

  1. Maybe we should support more complicated cases, not only skip. So I would suggest to enhance the inverse() API directly instead of bringing new APIs. And as we are still not clear about all the use cases, I think we can provide the basic feature to just specify a list of transforms to invert by class name or index. If no list, invert all.
  2. Could you please also help enhance the Invertd post transform? Almost all the users invert by this API.
  3. As Invertd is heavily used now, the AdjustContrast transform will cause invert ing error, we must fix this blocking issue ASAP.

@wyli @ericspod What do you think?

Thanks.

@Nic-Ma Nic-Ma closed this May 26, 2021
@Nic-Ma Nic-Ma reopened this May 26, 2021
@wyli

wyli commented May 26, 2021

Copy link
Copy Markdown
Contributor

shall we revert #2217 temporarily? we could include that after properly merging this PR.

@rijobro

rijobro commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

Sure, we can revert #2217, although I don't fully understand why we can't just temporarily manipulate image_transforms and label_transforms. If you remove AdjustContrastd from these lists, then they won't be inverted.

@rijobro

rijobro commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

@Nic-Ma I thought about modifying the inverse method, but the problem is that the interface should be the same for all InvertibleTransform classes, including Compose and all the others. The idea of skipping, however, only really makes sense for the Compose class, which is why I created a new method for it. Happy to discuss this tomorrow, though.

@rijobro

rijobro commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

Also @Nic-Ma I can't really modify Invertd until we make a decision here, since Invertd calls BatchInverseTransform, which eventually calls the inverse method. So first we need to agree on the inverse API before we can do Invertd.

@Nic-Ma

Nic-Ma commented May 26, 2021

Copy link
Copy Markdown
Contributor

Sure, let's discuss it online.

Thanks.

@madil90

madil90 commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

/build

2 similar comments
@madil90

madil90 commented Jul 19, 2021

Copy link
Copy Markdown
Contributor

/build

@madil90

madil90 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

/build

@wyli

wyli commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

this is addressed by Metatensor and the array level inverse.

@wyli wyli closed this Jul 19, 2022
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.

4 participants