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

Fix tanh activiation in pytorch parser #1055

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

JanFSchulte
Copy link
Contributor

@JanFSchulte JanFSchulte commented Aug 28, 2024

@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

  • Bug fix (non-breaking change that fixes an issue)

Tests

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@JanFSchulte JanFSchulte added the please test Trigger testing by creating local PR branch label Aug 28, 2024
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'
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is safe. I see the templates will lower() it here so the only way to break if it is not lowered is for Quartus which does this for god knows what reason. I think we generally expect it to be lowercase, so it should be fine to move it up to a line 45.

Copy link
Contributor Author

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.

@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 28, 2024
@vloncar vloncar enabled auto-merge (squash) August 28, 2024 16:27
@vloncar vloncar merged commit d63033b into fastmachinelearning:main Sep 11, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants