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

meaningful node naming when using from_list() #43

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

matjobst
Copy link
Collaborator

@matjobst matjobst commented Jul 27, 2023

Implementation for Issue #20
This should not break any end user code because it is just a modification on the naming of the nodes generated by NIRGraph.from_list().
Naming follows Tensorflow convention:
First occurence of Node type will get classname in lower case, from the second occurence a _<i> is appended, where <i> is the count of previous occurences.
Example:
input -> dense -> dense_1 -> affine -> dense_2 -> affine ->output

Copy link
Collaborator

@Jegp Jegp left a comment

Choose a reason for hiding this comment

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

There are a few print statements that should be removed. And I had fun trying to find other ways to find a unique_node_name with some Python collection types.

Otherwise, nice addition :)

nir/ir.py Outdated Show resolved Hide resolved
nir/ir.py Outdated Show resolved Hide resolved
nir/ir.py Outdated Show resolved Hide resolved
@matjobst
Copy link
Collaborator Author

Nicely spotted on the prints. They are removed now.
And thank you for the hint on the collections, I always forget about these. I implemented the Counter version now because it shows the intent of the code best.

@matjobst matjobst requested a review from Jegp July 28, 2023 07:39
Copy link
Collaborator

@Jegp Jegp left a comment

Choose a reason for hiding this comment

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

Nice! I like your way to do the format string with f"{}{}" it looks much better than f"{}" + ... :)

@matjobst matjobst merged commit e76fc00 into main Jul 28, 2023
5 checks passed
@matjobst matjobst deleted the feature_from_list_naming branch July 28, 2023 08:41
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