MIDASverse / MIDASpy

Python package for missing-data imputation with deep learning
Apache License 2.0
134 stars 37 forks source link

Use of ```isinstance``` instead of ```type``` #23

Closed David-Woroniuk closed 2 years ago

David-Woroniuk commented 2 years ago

Firstly, a great package.

I noticed that the package uses if type(var) == float:, and thought it may be useful to modify the behaviour to be more Pydantic.

To summarise, isinstance caters for inheritance (where an instance of a derived class is an instance of a base class), while checking for equality of type does not. This instead demands identity of types and rejects instances of subclasses.

Typical Python code should support inheritance, so isinstance is less bad than checking types, as it supports inheritance. However, “duck typing” would be the preferred (try, except), catching all exceptions associated with an incorrect type (TypeError).

I refer to lines 142-153, whereby the list type is evaluated:

    if type(layer_structure) == list:
      self.layer_structure = layer_structure
    else:
      raise ValueError("Layer structure must be specified within a list")

which could be achieved more elegantly using:

if not isinstance(layer_structure, list):
    raise TypeError("Layer structure must be specified within a list.")

181-187:

    if weight_decay == 'default':
      self.weight_decay = 'default'
    elif type(weight_decay) == float:
      self.weight_decay = weight_decay
    else:
      raise ValueError("Weight decay argument accepts either 'standard' (string) "\
                       "or floating point")

whereby the type (or types) could be hinted to the user within the init dunder method, and can be evaluated through:

if isinstance(weight_decay, str):
   if weight_decay != 'default':
        raise ValueError("A warning that the value must be 'default' or a float type")
   self.weight_decay = weight_decay
elif isinstance(weight_decay, float):
   self.weight_decay = weight_decay

Depending on the python versions supported, I would also recommend using typehints, and using the below:

from typing import List

abc_var: List[int]

More than happy to submit a PR with the proposed changes.

ranjitlall commented 2 years ago

Hi @David-Woroniuk, thanks for the excellent suggestion. It would be great if you could submit a PR request. And please do let us know if you have other ideas for improving the package!

David-Woroniuk commented 2 years ago

@ranjitlall - PR is with you now - please push through your unit tests ahead of merging.