[Refactor] Refactor the interface for RoIAlignRotated

[nijkah]

Motivation

RoIAlignRotated does not have the same interface as RoIAlign.

Modification

I edited its interface and added deprecated warning.

[CLAassistant]

All committers have signed the CLA.

[zhouzaida]

Hi @nijkah , thanks for your contribution. LGTM. BTW, it would be better to add a unit test for the renaming behavior. https://github.com/open-mmlab/mmcv/blob/master/tests/test_ops/test_roi_align_rotated.py

[nijkah]

@zhouzaida Actually, I'm not familiar with a unit test. What is the best convention to test this kind of behavior?
Should I define new function like (def _test_roialign_rotated_deprecated_parameters) or just add new forward & backward with new & deprecated parameters under the function _test_roialign_rotated_allclose?

[zhouzaida]

@zhouzaida Actually, I'm not familiar with a unit test. What is the best convention to test this kind of behavior? Should I define new function like (def _test_roialign_rotated_deprecated_parameters) or just add new forward & backward with new & deprecated parameters under the function _test_roialign_rotated_allclose?

The former way may be better.

[nijkah]

@zhouzaida Can you check the test code? Is the code you intended?

[zhouzaida]

@zhouzaida Actually, I'm not familiar with a unit test. What is the best convention to test this kind of behavior? Should I define new function like (def _test_roialign_rotated_deprecated_parameters) or just add new forward & backward with new & deprecated parameters under the function _test_roialign_rotated_allclose?

Maybe the latter will be more concise. 🤣

[nijkah]

@zhouzaida
Function _test_roialign_rotated_allclose tests the roi_align_rotated function not RoIAlignRotated Module.
Although function _test_roialign_rotated_gradcheck uses RoIAlignRotated Module, but it might seem odd to test deprecated parameters in that function.
Any suggestion? I am sorry to bother you. :(

[zhouzaida]

@zhouzaida Function _test_roialign_rotated_allclose tests the roi_align_rotated function not RoIAlignRotated Module. Although function _test_roialign_rotated_gradcheck uses RoIAlignRotated Module, but it might seem odd to test deprecated parameters in that function. Any suggestion? I am sorry to bother you. :(

Yet, we just need to test the deprecated parameters in _test_roialign_rotated_allclose.

[nijkah]

Option 1. Replace roi_align_rotated function with RoIAlignRotated Module in the _test_roialign_rotated_allclose.
Option 2. Just add a test code for RoIAlignRotated Module after for loop of _test_roialign_rotated_allclose like below

  for case, output in zip(inputs, outputs):
        np_input = np.array(case[0])
        np_rois = np.array(case[1])
        np_output = np.array(output[0])
        np_grad = np.array(output[1])
        x = torch.tensor(
            np_input, dtype=dtype, device=device, requires_grad=True)
        rois = torch.tensor(np_rois, dtype=dtype, device=device)
        output = roi_align_rotated(x, rois, (pool_h, pool_w), spatial_scale,
                                   sampling_ratio, True)
        output.backward(torch.ones_like(output))
        assert np.allclose(
            output.data.type(torch.float).cpu().numpy(), np_output, atol=1e-3)
        assert np.allclose(
            x.grad.data.type(torch.float).cpu().numpy(), np_grad, atol=1e-3)

  # end of original function

  roi_align_rotated_module_deprecated = RoIAlignRotated(
      out_size=(pool_h, pool_w),
      spatial_scale=spatial_scale,
      sample_num=sampling_ratio)

  output_v1 = roi_align_rotated_module_deprecated(x, rois)

  roi_align_rotated_module_new = RoIAlignRotated(
      output_size=(pool_h, pool_w),
      spatial_scale=spatial_scale,
      sampling_ratio=sampling_ratio)

  output_v2 = roi_align_rotated_module_new(x, rois)
  ssert np.allclose(
            output_v1.data.type(torch.float).cpu().numpy(),
            output_v2.data.type(torch.float).cpu().numpy())

[zhouzaida]

Option 1. Replace roi_align_rotated function with RoIAlignRotated Module in the _test_roialign_rotated_allclose. Option 2. Just add a test code for RoIAlignRotated Module after for loop of _test_roialign_rotated_allclose like below

  for case, output in zip(inputs, outputs):
        np_input = np.array(case[0])
        np_rois = np.array(case[1])
        np_output = np.array(output[0])
        np_grad = np.array(output[1])
        x = torch.tensor(
            np_input, dtype=dtype, device=device, requires_grad=True)
        rois = torch.tensor(np_rois, dtype=dtype, device=device)
        output = roi_align_rotated(x, rois, (pool_h, pool_w), spatial_scale,
                                   sampling_ratio, True)
        output.backward(torch.ones_like(output))
        assert np.allclose(
            output.data.type(torch.float).cpu().numpy(), np_output, atol=1e-3)
        assert np.allclose(
            x.grad.data.type(torch.float).cpu().numpy(), np_grad, atol=1e-3)

  # end of original function

  roi_align_rotated_module_deprecated = RoIAlignRotated(
      out_size=(pool_h, pool_w),
      spatial_scale=spatial_scale,
      sample_num=sampling_ratio)

  output_v1 = roi_align_rotated_module_deprecated(x, rois)

  roi_align_rotated_module_new = RoIAlignRotated(
      output_size=(pool_h, pool_w),
      spatial_scale=spatial_scale,
      sampling_ratio=sampling_ratio)

  output_v2 = roi_align_rotated_module_new(x, rois)
  ssert np.allclose(
            output_v1.data.type(torch.float).cpu().numpy(),
            output_v2.data.type(torch.float).cpu().numpy())

Option2 would be fine.

[zhouzaida]

Should we also refactor the interface of forward?

[nijkah]

@zhouzaida Do you mean forward of RoIAlignRotatedFunction class? I think it will be desirable to understand the interface.
If you add emoji to this comment, I will edit it.

[zhouzaida]

@zhouzaida Do you mean forward of RoIAlignRotatedFunction class? I think it will be desirable to understand the interface. If you add emoji to this comment, I will edit it.

Got it. BTW, we also need to update the docstring.

[nijkah]

@zhouzaida Do you mean forward of RoIAlignRotatedFunction class? I think it will be desirable to understand the interface. If you add emoji to this comment, I will edit it.

Got it. BTW, we also need to update the docstring.

I edited docstring.
And I also edited cuda and cpu interfaces to refactor the RoIAlignRotatedFunction.

[zhouzaida]

Please @luopeichao have a look.

[nijkah]

What should I do else for the next step? @zhouzaida

[zhouzaida]

What should I do else for the next step? @zhouzaida

Thanks for your contribution. I will merge the PR in a few days.

[nijkah]

@zhouzaida
Thanks for your reply. I apologize if I made you hurry. 😔
I just wished this PR will be merged soon since it makes mmdetection code for rotated objects messy.

[luopeichao]

Looks good to me about parrots extension changes.

[zhouzaida]

@zhouzaida Thanks for your reply. I apologize if I made you hurry. 😔 I just wished this PR will be merged soon since it makes mmdetection code for rotated objects messy.

Thanks for your contribution. I will merge the PR today.