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

add type check for input name of resolve_datatype() #121

Conversation

makoeppel
Copy link
Contributor

@makoeppel makoeppel commented Jun 21, 2024

I found some potential issue in the resolve_datatype() function of the datatype.py file:

def resolve_datatype(name):
    _special_types = {
        "BINARY": IntType(1, False),
        "BIPOLAR": BipolarType(),
        "TERNARY": TernaryType(),
        "FLOAT32": FloatType(),
        "FLOAT16": Float16Type(),
    }
    if name in _special_types.keys():
        return _special_types[name]
    elif name.startswith("UINT"):
        bitwidth = int(name.replace("UINT", ""))
        return IntType(bitwidth, False)
    elif name.startswith("INT"):
    ...

Looking at this function I first thought one has to pass a string for the name input. However, if the input name is of type for example qonnx.core.datatype.BipolarType with a name attribute which is covered in the _spacial_types dictonary (in this case Bipolar) the check if name in _special_types.keys(): will be true. But when you pass for example an qonnx.core.datatype.IntType class the check if name in _special_types.keys(): will be false and the program will crash at the next check elif name.startswith("UINT"): since here the assumption is that the input name is of type str. So long story short the first if can accept a str or a class which is named in the _spacial_types dictonary while the other cases only work with str.

I propose to introduce a type check for the input name for a better error handling. I also added a test case to check if the error is raised and another one to test the resolve_datatype function.

@maltanar
Copy link
Collaborator

Thanks @makoeppel ! For future PRs to qonnx please make sure you've run pre-commit: https://github.com/fastmachinelearning/qonnx?tab=readme-ov-file#linting

@maltanar maltanar merged commit 51965ab into fastmachinelearning:main Dec 20, 2024
5 checks passed
@makoeppel
Copy link
Contributor Author

Thank you @maltanar for merging! I normally do run them. But also for the other PRs they are failing online. I wanted anyway to do some cleanup before the year ends so I will have a look at the other PRs as well.

@maltanar maltanar added this to the v0.4.0 milestone Dec 20, 2024
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

Successfully merging this pull request may close these issues.

2 participants