Deprecate Old Pathology Components

[bhashemian]

Fixes #4226

This PR deprecates datasets and append the previous test cases with _deprecated.

Status

Ready

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).

[Nic-Ma]

Hi @drbeh ,

As discussed in #233 (comment), I think maybe we can:

1. Combine the tests for the latest module and the deprecated module in only one python file test_wsireader.py, same for test_patch_wsi_dataset.
2. We will drop the tests for deprecated module in the future version and give error message instead of warning message.

The benefit of this method is to keep tracking the change of tests in one python code file and easy to check the history.
What do you think?

Thanks.

[Nic-Ma]

Hi @ericspod @rijobro ,

What do you guys think about the idea?
Thanks in advance.

[rijobro]

I think this should be a note to ourselves not to call things _new, as they only stay new for a short while!

[ericspod]

Definitely should avoid using "new" or "old", when a feature is removed should be mandatory and we remove the feature and its tests at the stated version. Perhaps the tests to be removed should be marked as deprecated as well.

[Nic-Ma]

Definitely should avoid using "new" or "old", when a feature is removed should be mandatory and we remove the feature and its tests at the stated version. Perhaps the tests to be removed should be marked as deprecated as well.

Thanks @ericspod @rijobro ,

That's a good point!

@drbeh What do you think? Could you please help merge the test cases into one file and also mark the tests for deprecated APIs as "deprecated"?

Thanks in advance.

[bhashemian]

That is fine with me and I agree that we should not use words new and old. However, usually we don't encounter situations to have components with the same name but the problem arises because we called the test file of components in the apps folder similar to the ones in the core. It could have easily avoided by calling the test file for monai.apps.xxx.yyy, for instance, as test_xxx_yyy.py and reserve test_yyy.py for components in the core.

What do you think? @Nic-Ma @ericspod @rijobro @wyli

[Nic-Ma]

Hi @drbeh ,

As I described in the previous comment, I think one of the main benefits to merge the test cases is that: it can help keep tracking the change of tests in one python test code file and easy to check the history.
What do you think?

Thanks.

discussion in progress...

[bhashemian]

Hi @drbeh ,

As I described in the previous comment, I think one of the main benefits to merge the test cases is that: it can help keep tracking the change of tests in one python test code file and easy to check the history. What do you think?

Thanks.

Hi @Nic-Ma, maybe I am missing something but in this way we will lose the track of the tests for the newer component, don't we?

Anyways, if it makes this PR acceptable to merge, I'll do it because I don't think that it is that important. Just to clarify, when you say to mark it as deprecated, do you mean adding deprecated to the test name or use deprecated decorator?

[Nic-Ma]

Hi @drbeh ,

Sorry maybe I didn't describe it clearly.
I think @ericspod @rijobro and I were talking about this idea:

1. Write all the tests for both the deprecated component and the new component in the same python file.
2. Mark the tests for the deprecated component with deprecated decorator (maybe defining the test program for the deprecated component in a unique class with a decorator).

What do you think?
@ericspod @rijobro Please feel free to correct me if I misunderstand anything.

Thanks in advance.

[bhashemian]

Hi, I have combined the unittest files according to what we have discussed. Please let me know if you have any other comment.

[wyli]

Thanks, it looks good to me.

[wyli]

/build

[Nic-Ma]

Thanks for the quick update!
Looks good to me.

[wyli]

/build