-
Notifications
You must be signed in to change notification settings - Fork 421
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
Fix tanh activiation in pytorch parser #1055
Conversation
hls4ml/converters/pytorch/core.py
Outdated
if layer['class_name'] == 'ReLU' or layer['class_name'] == 'Sigmoid': | ||
if layer['class_name'] in ['ReLU', 'Sigmoid', 'Tanh']: | ||
if layer['class_name'] == 'Tanh': | ||
layer['activation'] = 'tanh' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to do this for tanh
but not for the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a layer['activation'] = layer['class_name']
a little bit above in the code, but for tanh
we have to make it a lower case t
. I guess that could be done just with an layer['activation'] = layer['class_name'].lower()
, but I'm not sure if it's a general rule that the activation
attribute is supposed to be lower case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's exactly because of this stuff in Quartus why I had to add this clause in the first place. But just making the attribute lower case works, so I changed it to that.
@rianbrooksflynn noticed that tanh activtivation was listed in the supported activations in the pytorch parser, but not actually parsed properly. Fixed in this PR and added to the pytests.
Type of change
Tests
Checklist
pre-commit
on the files I edited or added.