Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of isinstance instead of type #23

Closed
David-Woroniuk opened this issue Jul 5, 2022 · 2 comments
Closed

Use of isinstance instead of type #23

David-Woroniuk opened this issue Jul 5, 2022 · 2 comments

Comments

@David-Woroniuk
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants