deepchecks icon indicating copy to clipboard operation
deepchecks copied to clipboard

[draft] fix logic

Open yromanyshyn opened this issue 1 year ago • 0 comments

@nirhutnik

few thoughts/suggestions

Point 1.

IMO, we should create context instances in the same way and with the same defaults as how base checks do that. We need to separate that logic from base checks run methods into separate functions that will be reused everywhere, in one of the ways:

# == Separate base check methods ==

class SingleDatasetCheck(...):
    def prepare_context(self, ...):
         return ...
    @merge_args(SingleDatasetCheck.prepare_context)     
    def run(self, *args, **kwargs):
         context = self.prepare_context(*args, **kwargs)
         ...

# or
# == Context factory methods ==

class Context:
    @classmethod
    def for_single_dataset_check(cls, ...):
         ...

class SingleDatasetCheck(...):
    @merge_args(Context.for_single_dataset_check)
    def run(self, *args, **kwargs):
         context = Context.for_single_dataset_check(*args, **kwargs)
         ...

Point 2.

SingleDatasetCheckFixMixin and TrainTestCheckFixMixin are not needed at all, at least currently, because you can just do isinstance(check, (SingleDatasetCheck, TrainTestCheck))

Point 3.

I would suggest moving the 'fix' logic into a separate sub-package in order to not overload check classes with too much logic, something like

# fix sub-package

class MixedNullsFix:
    def __init__(self, some_arg):
       self.some_arg = some_arg
    def fix(self, context):
       ...

# check module

class MixedNullCheck(...):
   @merge_args(...)
   def fix(self, *args, some_arg: Any = None, **kwargs):
      from ... import MixedNullFix
      context = self.prepare_context(*args, **kwargs)
      fix = MixedNullFix(some_args)
      return fix.fix(context)

but to be honest, not sure about it yet, most of the fix logic currently looks pretty simple

yromanyshyn avatar Jun 26 '23 05:06 yromanyshyn