deepchecks icon indicating copy to clipboard operation
deepchecks copied to clipboard

1848 rename whole dataset drift check to multivariate drift

Open kishore-s-15 opened this issue 2 years ago • 2 comments

Closes #1848

kishore-s-15 avatar Aug 01 '22 16:08 kishore-s-15

@ItayGabbay @Nadav-Barak @shir22 Hi Guys, Any updates on this PR 🙂 ?

kishore-s-15 avatar Aug 08 '22 11:08 kishore-s-15

@kishore-s-15 Hey! After we noticed there are some users who are using the ImageDatasetDrift, I think we shouldn't remove it. In addition, after reviewing your PR I suggest that we should support the old name (WholeDatasetDrift) but with a deprecation warning. Exactly like we did in the SegmentPerformance check.

ItayGabbay avatar Aug 08 '22 14:08 ItayGabbay

@kishore-s-15 Hey! After we noticed there are some users who are using the ImageDatasetDrift, I think we shouldn't remove it. In addition, after reviewing your PR I suggest that we should support the old name (WholeDatasetDrift) but with a deprecation warning. Exactly like we did in the SegmentPerformance check.

I'll make the necessary changes.

kishore-s-15 avatar Aug 17 '22 11:08 kishore-s-15

@kishore-s-15 Hey! After we noticed there are some users who are using the ImageDatasetDrift, I think we shouldn't remove it. In addition, after reviewing your PR I suggest that we should support the old name (WholeDatasetDrift) but with a deprecation warning. Exactly like we did in the SegmentPerformance check.

I have looked into the SegmentPerformance check code file. I'll make similar changes to WholeDatasetDrift check. Could you mention what's the version number I need to specify for depreciation and also for removal?

kishore-s-15 avatar Sep 08 '22 10:09 kishore-s-15

Hi @kishore-s-15 !

For removal you can specify version number 0.11. Deprecated - since the current version of when this PR will be merged, which depends... (the next version to be released is 0.9, and it will be released this week).

By the way - notice that PR's like this that touch multiple existing files that are many times being updated, it would be best to try finalize it in one shot (with quick fixes so that we can have it merged into main in a day or so) otherwise we'll have to keep merging from main and dealing manually with the new conflicts (like currently - there are many changes that have to be updated, it might even be easiest working on a new completely pull from main..). If you prefer that someone else assist you in finalizing this issue due to the nature of this PR, feel free to state so, we'll be happy to do it.

One small note for best practice - make sure to "rename" the renamed files using git (so that history is tracked), instead of rename/move them using your operating system, cause then git will consider that a deleted + new file instead.

shir22 avatar Sep 12 '22 05:09 shir22

Sure, @shir22. As mentioned, I'll specify the deprecation version as 0.9 & removal version as 0.11.

So far in this PR, I have removed ImageDatasetDrift check & renamed the WholeDatasetDrift (in a lot of files). I'll close this PR and start working on a new branch from the latest main branch.

I never knew git mv existed previously, I've always renamed the files using the operation system. Thanks for letting me know of this feature, it'll be really helpful for me. I'll rename files using it in the future.

kishore-s-15 avatar Sep 12 '22 12:09 kishore-s-15

Moved to https://github.com/deepchecks/deepchecks/pull/2011

kishore-s-15 avatar Sep 14 '22 14:09 kishore-s-15