search

nikhilk / node-tensorflow

Node.js + TensorFlow
Apache License 2.0
587 stars 59 forks source link

Alias and Name mixed up in graph.js #18

Closed josiahkhor closed 6 years ago

josiahkhor commented 6 years ago

It seems that the alias and name definitions are the wrong way around in loadOperations()? I think it should be

for (let alias in nameAliasMap) { let name = nameAliasMap[alias];

nikhilk commented 6 years ago

When I coded it (and unfortunately didn't yet document it), I was expecting the user to supply a dictionary containing full tensor names mapped to alias names ... at least based on that description.

So something like:

{ Const_1: 'c', 'namescope:Const_2': 'c2' }

That sort of thing.

Does the alternate description sound better? A list of aliases along with what op they refer to? Entirely possible that is more intuitive/natural API. Please share thoughts.

In which case the parameter name would change to aliases and indeed we'd switch the code accordingly.

Edit: as I think about it, yes, aliases -> op names sounds better. As added bonus, aliases will generally be valid identifiers, so the map can be specified without keys in quotes.

nikhilk commented 6 years ago

Addressed in e48c34805f196ecbd72a8e61462225209d16f0fe Also, changes published in 0.6.5 package on npm and added an accompanying sample (see graph/matrix sample).