Re: [w3ctag/design-reviews] Web Neural Network API (#570)

@kenchris and I looked this today.

First-pass review - we have a bunch of questions:

1. The fact that a GRU is in there really sticks out. I somehow found out why it is there, but it feels extremely inconsistent with the rest of the API which is fairly generic. (e.g. you should have a LSTM and a GRU, but not just a GRU - that's weird.) 
2. In the spec, some of the activations are out in global scope (e.g. relu), some are in unary operators (sigmoid, tanh) - this doesn't look consistent.
3. The spec mentions training in the batch normalization section - but I'm fairly convinced that there is no support for training. Is this an error?
4. getNeuralNetworkContext() and createModelBuilder() seem strange (no parameters, for one thing) - is this expected to accept parameters/configs at some point? If so, we'd like to see what is intended here.
5. Wouldn't it make sense to have a constructor rather than a builder pattern for createModelBuilder()? (e.g. new ModelBuilder(navigator.ml.getNNContext());
6. I see quite a few view/reshape like functions, which of these are expected to copy and which are not? Probably good to note this in the spec.
7. If there are layers that will be taking activations as string enums, there should simply be a string enum for activations rather than have it just in RecurrentNetworkActivation. (One may argue that hyperbolic tangent is RNN specific, but...)
8. While the limitations of JavaScript probably contribute a lot to this, but the ergonomics of this API based on example code might have room for improvement.
9. It feels like errors/exceptions should probably fleshed out. (e.g. what happens when you try to reduce on a non-existent axis?)
10. I don't quite understand the NamedOutput mechanism. What if what is output just a feature?
11. A lot of the names are very generic (Operand, Compilation) - this feels like something we might want to prefix with something or synchronize with TC39 about.
12. What's the isomorphic JS story for this? Also, given that this is attached to vanilla navigator, is this not expected to work in a worker scope?
13. Given that bootstrapping a network is a lot of work, would it make sense to have some sort of serialization/caching story here?

Nits:
1. The one case I saw clamp() being used seemed to implement a relu?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3ctag/design-reviews/issues/570#issuecomment-768875996