1977 Support different inverse interpolation mode

[Nic-Ma]

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]

This PR depends on: #1970

Thanks.

[Nic-Ma]

/black

[Nic-Ma]

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

Thanks in advance.

[Nic-Ma]

/integration-test

[rijobro]

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]

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]

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]

Hi @rijobro ,

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

[Nic-Ma]

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]

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]

/integration-test

[Nic-Ma]

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.

[rijobro]

@Nic-Ma looks great, thanks!