Skip to content

1977 Support different inverse interpolation mode #1978

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.

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

Merged
merged 21 commits into from
Apr 9, 2021

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Apr 8, 2021

Fixes #1977

Description

This PR added support to use different interpolation mode in inverse transforms.

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 Nic-Ma marked this pull request as ready for review April 8, 2021 18:37
@Nic-Ma Nic-Ma changed the title [WIP] 1977 Support different inverse interpolation mode 1977 Support different inverse interpolation mode Apr 8, 2021
@Nic-Ma Nic-Ma requested review from rijobro, wyli and ericspod April 8, 2021 18:37
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 8, 2021

This PR depends on: #1970

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

the dependency PR had been merged.
Ready for review.
@wyli @rijobro could you please help review it?

Thanks in advance.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

/integration-test

@rijobro
Copy link
Contributor

rijobro commented Apr 9, 2021

Could the same be achieved with a withable (which wouldn't require adding mode to EXTRA_INFO)?

with nearest_neighbour_transforms(*transforms):
    # if Compose, get list (same logic as in allow_missing_keys_mode)
    all_transforms = ...

    # get sublist of all transforms that contain `mode`. 
    # might need recursion for transforms that contain transforms.
    mode_transforms = [i for i in all_transforms if hasattr(i, mode)]

    # record previous values
    orig_modes = [i.mode for i in mode_transforms]

    try:
        # Set all to nearest_neighbour
        for i in mode_transforms:
            i.mode = 'nearest-neighbour'
        yield
    finally:
        # Revert
        for t, o_s in zip(mode_transforms, orig_modes):
            t.allow_missing_keys = o_s

Could even be generalised to temp_modify_transforms(transforms, key, value) where in this case key would be mode and value would be nearest_neighbour.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

Hi @rijobro ,

Thanks for your suggestion.
Actually, @wyli and I discussed this approach yesterday, the benefit of my current solution is to save mode, align_corners, etc. into the dict which only affects current input data. It's dangerous to modify self.mode property in the transform instances and especially thread-safe related issues. We may support inverting several data in parallel later, some of them may expect nearest mode but some may not.
And we can also use this saved mode information for some future features.
@wyli what do you think?

Thanks.

@rijobro
Copy link
Contributor

rijobro commented Apr 9, 2021

Ok that sounds like good motivation for doing it your way. You've put the logic of modifying EXTRA_INFO inside of the ignite handler, but it could be put somewhere else for more general use. For example, we could allow the inverse method to accept a dictionary that could replace items in EXTRA_INFO. I think that would have the same effect as your current code but be accessible for users not using ignite:

def inverse(self, data, **extra_info):
    d = deepcopy(dict(data))
    for key in self.key_iterator(d):
        # replace any elements in d["EXTRA_INFO"] with **extra_info
        ...

and then we'd call it with transforms.inverse(data, {"mode": "nearest"})

What do you think?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

Hi @rijobro ,

Thanks for your suggestion, do you mean to add the replacement logic for every ItertibleTransform?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

Hi @rijobro ,

I think we can make the logic into a util function, then users can use it without ignite, what do you think?

Thanks.

@wyli
Copy link
Contributor

wyli commented Apr 9, 2021

Ok that sounds like good motivation for doing it your way. You've put the logic of modifying EXTRA_INFO inside of the ignite handler, but it could be put somewhere else for more general use. For example, we could allow the inverse method to accept a dictionary that could replace items in EXTRA_INFO. I think that would have the same effect as your current code but be accessible for users not using ignite:

def inverse(self, data, **extra_info):
    d = deepcopy(dict(data))
    for key in self.key_iterator(d):
        # replace any elements in d["EXTRA_INFO"] with **extra_info
        ...

and then we'd call it with transforms.inverse(data, {"mode": "nearest"})

What do you think?

I feel this is necessary for general usability, but we should do it in a separate PR as a new feature implementation? perhaps we don't have time for this new feature in v0.5...

@wyli
Copy link
Contributor

wyli commented Apr 9, 2021

/integration-test

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 9, 2021

Hi @rijobro and @wyli ,

I already changed the replacement logic to util function and added unit tests.
Now users can use it without the ignite workflows.
Could you please help review it again?

Thanks in advance.

Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma Nic-Ma enabled auto-merge (squash) April 9, 2021 13:36
@wyli wyli disabled auto-merge April 9, 2021 13:41
@rijobro
Copy link
Contributor

rijobro commented Apr 9, 2021

@Nic-Ma looks great, thanks!

@wyli wyli merged commit d178706 into Project-MONAI:master Apr 9, 2021
nsrivathsa pushed a commit to nsrivathsa/MONAI that referenced this pull request Apr 12, 2021
* [DLMED] add TransformInverter handler

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix typo

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add support in SegmentationSaver handler

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix flake8 issue

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix flake8 issue

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix CI test

Signed-off-by: Nic Ma <[email protected]>

* [MONAI] python code formatting

Signed-off-by: monai-bot <[email protected]>

* [DLMED] save mode into inverse dict

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add unit tests

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix ToTensor inverse issue

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] change the replacement logic into util function

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add more tests

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix flake8

Signed-off-by: Nic Ma <[email protected]>

* [MONAI] python code formatting

Signed-off-by: monai-bot <[email protected]>

Co-authored-by: monai-bot <[email protected]>
Signed-off-by: Neha Srivathsa <[email protected]>
@Nic-Ma Nic-Ma deleted the 1977-inverse-interpolation-mode 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.

Need to support different interpolation mode in inverse transforms
4 participants