This PR depends on: #1970

Thanks.

/black

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

Thanks in advance.

/integration-test

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.

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.

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?

Hi @rijobro ,

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

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.

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...

/integration-test

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.

@Nic-Ma looks great, thanks!