Skip to content

Conversation

@psunn
Copy link
Contributor

@psunn psunn commented May 15, 2024

Fixes a naming mismatch in MSCGraph where tensor_name could formatted as 'string:index:index',and the corresponding node.name is 'string:index'. Splitting tensor_name from the right aligns it correctly.

For example, the TFLite default input name 'serving_default_input:0' becomes 'serving_default_input:0:0' in MSCGraph, while node.name remains 'serving_default_input:0'.

Fixes a naming mismatch in MSCGraph where tensor_name could formatted as
'string:index:index',and the corresponding node.name is 'string:index'.
Splitting tensor_name from the right aligns it correctly.

For example, the TFLite default input name 'serving_default_input:0'
becomes 'serving_default_input:0:0' in MSCGraph, while node.name remains
'serving_default_input:0'.
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @psunn, would it be possible to add a test case?

@Hzfengsy
Copy link
Member

cc @Archermmt

@psunn
Copy link
Contributor Author

psunn commented May 16, 2024

Thanks for the fix @psunn, would it be possible to add a test case?

@lhutton1 Thanks for the review. I've added a test case.

@psunn
Copy link
Contributor Author

psunn commented May 22, 2024

Hi @Archermmt, could you help to review this PR? thanks in advance.

@Archermmt
Copy link
Contributor

Seem ok for me. The only change in this PR is the split direction, which has no influence on current test. I think the main reason is that: in some corner cases like TFLite model, the node name looks like "NODE:IDX", which is normally the tensor name format.

@psunn psunn force-pushed the fix_msc_split_str branch from c022eab to 6a1807f Compare May 28, 2024 12:28
@psunn
Copy link
Contributor Author

psunn commented May 28, 2024

@Archermmt thanks for review.
Yes, the change in this PR has no effect on existing tests. An additional test case has been added to cover node name_string with an index by colon.

I force-pushed only to address linting issues.

@lhutton1 @Archermmt @Hzfengsy
Would it be okay to merge it? thanks.

@psunn psunn force-pushed the fix_msc_split_str branch from 6a1807f to 00bb4e5 Compare May 28, 2024 13:18
@lhutton1 lhutton1 merged commit 7afac14 into apache:main May 29, 2024
@lhutton1
Copy link
Contributor

Thanks @psunn @Archermmt @Hzfengsy!

@psunn psunn deleted the fix_msc_split_str branch May 29, 2024 09:35
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.

4 participants