Refactor onnx math #187
Refactor onnx math #187
Conversation
| @@ -307,7 +307,7 @@ private ONNXOperators(String value, int numInputs, int numOptionalInputs, int nu | |||
| for (String o : outputs) { | |||
| nodeBuilder.addOutput(o); | |||
| } | |||
| nodeBuilder.setName(context.generateUniqueName(opName)); | |||
| nodeBuilder.setName(context.generateUniqueName(opName) + ":" + outputs[0]); | |||
There was a problem hiding this comment.
ONNX names are required by the spec to be alphanumeric + underscores.
| return OnnxMl.TensorProto.newBuilder() | ||
| .setName(context.generateUniqueName(name)) | ||
| .setDataType(OnnxMl.TensorProto.DataType.FLOAT.getNumber()) | ||
| .addAllDims(() -> dims.stream().map(Integer::longValue).iterator()) |
There was a problem hiding this comment.
I think I'd prefer this to just collect(Collectors.toList()) rather than make a lambda to fabricate the iterable. Plus in 17 we can replace it with .toList() which is shorter.
There was a problem hiding this comment.
Or even just make it accept List<Long> as that's what ONNX is expecting, and the upcast on the calling side when creating the list might be done automatically?
| ? floatTensorBuilder(context, name, Collections.singletonList(parameters.length), | ||
| fb -> Arrays.stream(parameters).forEachOrdered(d -> fb.put((float)d))) | ||
| : doubleTensorBuilder(context, name, Collections.singletonList(parameters.length), | ||
| db -> Arrays.stream(parameters).forEachOrdered(db::put)); |
There was a problem hiding this comment.
No 5 line ternary operators. An if statement with multiple returns is fine.
| */ | ||
| public static OnnxMl.TensorProto floatVectorBuilder(ONNXContext context, String name, SGDVector vector) { | ||
| return floatTensorBuilder(context, name, Collections.singletonList(vector.size()), | ||
| fb -> vector.forEach(vt -> fb.put(vt.index,(float) vt.value))); |
There was a problem hiding this comment.
I think it might be nicer to put the type on fb here. (FloatBuffer fb) -> ... means it's immediately obvious what's going on, otherwise you have to go look at floatTensorBuilder to figure out the inferred type. Ditto for the rest of the times this idiom appears in this file.
| DenseVector[] denseWeights = new DenseVector[weights.length]; | ||
| for (int i = 0; i < denseWeights.length; i++) { | ||
| denseWeights[i] = weights[i].densify(); | ||
| } |
There was a problem hiding this comment.
I think denseWeights should be pulled out of this lambda and be a local variable. It feels like a lot of magic to do in the lambda. Or even just make it into a DenseSparseMatrix and pass that in to floatMatrixBuilder. It would be a little easier to see what's going on then.
|
Could you rebase this PR to get rid of all the factorization machines commits it pulled in again? |
Craigacp
added
Oracle employee
JackSullivan
force-pushed
the
refactor-onnx-math
branch
from
8800de5 to
56db2fe
Compare
Description
As we add
ONNXExportableto new models, there is a bunch of duplicated logic on creatingOnnxML.TensorProtoinstances. This PR creates (hopefully) fully general methods for generating TensorProtos and replaces existing implementations with them.