Skip to content

Conversation

@wyli
Copy link
Contributor

@wyli wyli commented Jul 27, 2022

Signed-off-by: Wenqi Li wenqil@nvidia.com

Fixes #4769

Description

  • adds a util reset_ops_id to reset the id of the applied operation
  • enable it in the persistentcache datasets
    cc @SachidanandAlle

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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli requested review from Nic-Ma, ericspod and rijobro July 27, 2022 21:35
@Nic-Ma Nic-Ma requested a review from KumoLiu July 28, 2022 03:58
@wyli
Copy link
Contributor Author

wyli commented Jul 28, 2022

/build

@wyli wyli enabled auto-merge (squash) July 28, 2022 06:54
@wyli wyli disabled auto-merge July 28, 2022 11:11
Copy link
Contributor

@SachidanandAlle SachidanandAlle 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.. useful for monailabel as well.. it needs to cache image MetaTensor and to resuse in the pipeline it needs to reset the ids to run invert post tranforms

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jul 29, 2022

Hi @wyli @SachidanandAlle @rijobro @KumoLiu ,

Instead of permanently reseting the op ids in the applied_transforms, I would suggest to:

  1. Provide a module flag with the set/get APIs to enable / disable the ID checking, similar to our MetaTensor:
    https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/meta_obj.py#L30
  2. Then update the default value of check arg to check=get_id_check():
def pop_transform(self, data, key: Hashable = None, check: bool = get_id_check())

https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/inverse.py#L191

The benefit is that:

  1. The applied_transform information is still there.
  2. If some spetial transform must work with the original instance, the developer can explicitly set pop_transform(check=True) in its implementation.

What do you guys think?
Maybe I misunderstood the initial feature request, open for discussion.

Thanks.

@wyli
Copy link
Contributor Author

wyli commented Jul 29, 2022

Hi @Nic-Ma in my understanding when we store the applied operations info in a persistent cache, the id info becomes invalid because the cache is supposed to be loaded in other runs. In this case resetting the ids would be good. I'd be reluctant to change the pop_transform API unless we see a good use case for it. Note for some transforms there's also an 'inverse_transform' method to 'bypass' the check

return self.inverse_transform(data, transform)
I also don't see a general requirement of providing 'inverse_transform' API for every transform.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jul 29, 2022

Hi @wyli ,

I think users usually use persistent dataset for 2 reasons:
(1) Cache in the persistent storage, then reuse it in other runs.
(2) Cache in the persistent storage due to leak of memory storage in the typical training, then every epoch can reuse it.

For (1) you are right, the info becomes invalid, for (2) the applied transforms are still the original instances.
May I know what's the drawback of my module flag idea?
@rijobro @ericspod What do you guys think?

Thanks.

@wyli
Copy link
Contributor Author

wyli commented Jul 29, 2022

I think setting the data id would have finer control of the check. There's nothing wrong with your global flag, it might be useful in the future to completely turn off the check, but it's not needed at the moment. I think we should avoid global flags whenever possible.

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Copy link
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.

Thanks for the quick update.
Looks good to me now.

@wyli
Copy link
Contributor Author

wyli commented Aug 1, 2022

/build

@wyli wyli enabled auto-merge (squash) August 1, 2022 13:31
@wyli wyli merged commit 3a8c421 into Project-MONAI:dev Aug 1, 2022
@wyli wyli deleted the 4769-set-no-id branch August 4, 2022 13:21
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.

No option to skip the check when calling inverse method on the transform

3 participants