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

Top-level functions are shadowed by sub-module funcs #109

Merged
merged 5 commits into from
Jul 8, 2020

Conversation

vvmnnnkv
Copy link
Member

@vvmnnnkv vvmnnnkv commented Jul 6, 2020

Description

I've noticed that in torch -> tfjs conversion, tf.reshape is overridden by tf.layers.reshape, which has different arguments.
Correct translation would be torch.reshape -> tf.reshape, but tf.layers.reshape has same key in the funcs map and overwrites tf.reshape when the map is populated.
I'm trying to solve that by giving priority to functions that has shorter attrs path.

This kinda works, but it seems the problem needs more correct fix that would keep both funcs in the map but differentiate them by sub-module on mapping time?

Affected Dependencies

n/a

How has this been tested?

Unit tests

Checklist

@vvmnnnkv vvmnnnkv added Type: Bug 🐛 Some functionality not working in the codebase as intended Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes labels Jul 6, 2020
@Nolski
Copy link
Collaborator

Nolski commented Jul 6, 2020

This is related to #16

@vvmnnnkv vvmnnnkv requested a review from Nolski July 6, 2020 22:42
@vvmnnnkv
Copy link
Member Author

vvmnnnkv commented Jul 7, 2020

Noticed issue in tfjs parsed .json file:
e.g. if you see tf.GraphModel in tfjs API, it's methods are listed w/o tf.GraphModel. prefix.
Doc parser doesn't add tf.GraphModel prefix, so all these methods appear as is they were in global scope.
I'm going to manually remove "add" from tfjs/1.5.1.json because it interferes with tf.add(), but will create separate issue to fix doc parser.

@vvmnnnkv
Copy link
Member Author

vvmnnnkv commented Jul 7, 2020

Separate ticket: #113

Copy link
Collaborator

@Nolski Nolski left a comment

Choose a reason for hiding this comment

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

@Nolski Nolski merged commit ccd163a into master Jul 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/dont-override-base-funcs branch July 8, 2020 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes Type: Bug 🐛 Some functionality not working in the codebase as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants