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

remove cppname #562

Merged
merged 2 commits into from
Jul 12, 2022
Merged

remove cppname #562

merged 2 commits into from
Jul 12, 2022

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Jun 2, 2022

Variable name and cppname are often used interchangeably in the code, not consistently. My understanding is that cppname was meant to be a cleaned up C-legal version of name. Instead of looking at each case where name is used and trying to see if it should be cppname and vice versa, it probably makes sense to just get rid of cppname altogether, and just use name everywhere.

One question is whether we want to include this name cleaning in the Variable.__init__ function:

self.name = re.sub(r'\W|^(?=\d)','_', self.name)

I have not included it. The standard pytests all pass without it.

@vloncar
Copy link
Contributor

vloncar commented Jun 2, 2022

Cleaning the name at that point would in fact break the model if a change actually happened. cppname dates back to when the output variable names followed the layer names, so naming a layer "my layer" would generate invalid C++. Since we switched to using layer{index}_out as the name of the output variable in generated C++, this is no longer relevant so cppname can be removed. I forgot to do it as part of the backend restructuring. Thanks for this.

@jmitrevs
Copy link
Contributor Author

jmitrevs commented Jun 3, 2022

For Quartus we rename name for structure variables (the input and output). Is that safe?

@jmitrevs
Copy link
Contributor Author

I would like if possible to have this merged fairly soon since it effects new developments, which have to consider this PR.

@vloncar
Copy link
Contributor

vloncar commented Jul 12, 2022

This looks fine to me, I tried it on my Vivado developments and nothing breaks. But Quartus worries me a bit. I remember (ab)using cppname there for something. @bo3z Can you try this out and if there are no issues we merge it ASAP?

@bo3z
Copy link
Contributor

bo3z commented Jul 12, 2022 via email

@vloncar vloncar merged commit db943b7 into fastmachinelearning:master Jul 12, 2022
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
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.

3 participants