Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Evaluate the opportunity to replace gonum.DirectedWeightedBuilder in the top level Model #8

Closed
owulveryck opened this issue Mar 26, 2019 · 3 comments
Assignees
Labels
API Break enhancement New feature or request
Milestone

Comments

@owulveryck
Copy link
Owner

The Model is an encapsulation of gonum.DirectedWeightedBuilder's interface.

This gives some great flexibility for implementing a backend.
For example, you can create a very simple backend (see the example in "simple") simply to display the graph in case you do not need actually to compute the result.
For example, I am using this to create a dot representation of the onnx graph.

If the backend needs more feature, the backend can fulfill this interface, and the corresponding method is applied at runtime :

type OperationCarrier interface {
        ApplyOperation(Operation, graph.Node) error
}

The problem is that a type inference is made at runtime and this process is slow.

Proposal: I am considering to replace the embedded gonum.DirectedWeightedBuilder by a Graph interface that would look like:

type Graph interface {
       gonum.DirectedWeightedBuilder
       OperationCarrier
}

This would break the API which is, by now, not a problem.
What do you think?

@owulveryck owulveryck changed the title Evaluate the opportunity to replace gonum.DirectedWeightedBuilder in Model Evaluate the opportunity to replace gonum.DirectedWeightedBuilder in the top level Model Mar 26, 2019
@owulveryck owulveryck self-assigned this Mar 26, 2019
@owulveryck
Copy link
Owner Author

Work in progress, see https://github.com/owulveryck/onnx-go/tree/new-interface

@owulveryck owulveryck added API Break enhancement New feature or request labels Mar 26, 2019
@owulveryck owulveryck added this to the v0.2 milestone Mar 26, 2019
@owulveryck
Copy link
Owner Author

We should also have a look at the Operation type which is using a pointer to pb.AttributeProto; but this is declared inside an internal package...

@owulveryck
Copy link
Owner Author

Here is my current proposal.

I create a new interface:

// AttributesUnmarshaler is the interface implemented by types that can unmarshal  to
// themselves. AttributesUnmarshaler must copy the data if it wished to retain the data after returning.
type AttributesUnmarshaler interface {
        UnmarshalONNXAttributes(map[string]interface{}) error
}

The map[string]interface{} is a key value of the attributes; ex:

map[string]interface{}{
       "pad": []int{1,1},
      "kernel": tensor.Dense{},
}

It's simple for the type to check whether and expected value is present in the map, and the type inference shouldn't be compliated.

On top of that, I may keep this method private:

func unmarshalAttributes(attrs []*pb.AttributeProto, v interface{}) error {
        return pb.UnmarshalAttributes(attrs, v)
}

When decoding the attributes, I will first check if the Operator is fulfilling AttributesUnmarshaler, otherwise a call to unmarshalAttributes is performed. unmarshalAttributes will use the tags of the operator and this should be carefully documented.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Break enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant