catalyst: Naming inconsistency

Describe the bug I found that some names agruments in framework aren’t consistent. So for example:

class SupervisedRunner(Runner):
    """Runner for experiments with supervised model."""

    _experiment_fn: Callable = SupervisedExperiment

    def __init__(
        self,
        model: Model = None,
        device: Device = None,
        input_key: Any = "features", 
        output_key: Any = "logits",
        input_target_key: str = "targets", # This argument corresponds to input_key argument in CriterionCallback
    ):

class CriterionCallback(_MetricCallback):
    """Callback for that measures loss with specified criterion."""

    def __init__(
        self,
        input_key: Union[str, List[str], Dict[str, str]] = "targets", # This argument corresponds to input_target_key argument in SupervisedRunner
        output_key: Union[str, List[str], Dict[str, str]] = "logits",
        prefix: str = "loss",
        criterion_key: str = None,
        multiplier: float = 1.0,
        **metric_kwargs,
    ):

To Reproduce Steps to reproduce the behavior:

Check files: catalyst.core.callback.metric.py and catalyst.dl.runner.supervised.py

Expected behavior I expect that names would be consistent across the framework and means the same

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 17 (14 by maintainers)

Most upvoted comments

How it now

input_target_key corresponds to input_key argument in CriterionCallback input_target_key: str = "targets"

input_key corresponds to input_target_key argument in SupervisedRunner input_key: Union[str, List[str], Dict[str, str]] = "targets"

class SupervisedRunner(Runner):
    """Runner for experiments with supervised model."""

    _experiment_fn: Callable = SupervisedExperiment

    def __init__(
        self,
        model: Model = None,
        device: Device = None,
        input_key: Any = "features", 
        output_key: Any = "logits",
        # !!!input_target_key corresponds to input_key argument in CriterionCallback!!!
        input_target_key: str = "targets", 
    ):

class CriterionCallback(_MetricCallback):
    """Callback for that measures loss with specified criterion."""

    def __init__(
        self,
        # !!!input_key corresponds to input_target_key argument in SupervisedRunner!!!
        input_key: Union[str, List[str], Dict[str, str]] = "targets",         
        output_key: Union[str, List[str], Dict[str, str]] = "logits",
        prefix: str = "loss",
        criterion_key: str = None,
        multiplier: float = 1.0,
        **metric_kwargs,
    ):

How it should

Since CriterionCallback calcucates criterion between target (or input_target_key) and output_key it should be better to rename input_key to input_target_key as in the SupervisedRunner


class CriterionCallback(_MetricCallback):
    """Callback for that measures loss with specified criterion."""

    def __init__(
        self,
        # previous version
        input_key: Union[str, List[str], Dict[str, str]] = "targets", 
        # suggested version
        input_target_key: Union[str, List[str], Dict[str, str]] = "targets",         
        ...
    ):

Also I suggest to check this across whole framework

ogvalt on Apr 27, 2020