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

Add NNEF support for copy operation #1318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mmagician
Copy link

For a .nnef model such as:

graph.nnef:

version 1.0;

graph main(external1) -> (copy1)
{
    external1 = external<scalar>(shape = [1, 28, 28]);
    copy1 = copy(external1);
}

graph.quant:

"external1": zero_point_linear_quantize(zero_point = 0, scale = 0.003921568859368563, bits = 8, signed = false, symmetric = false);
"copy1": zero_point_linear_quantize(zero_point = -128, scale = 0.003921568859368563, bits = 8, signed = true, symmetric = false);

Let me know if I should include a sample .nnef model for testing somewhere?

element_wise!(copy, Copy, [i8, i16, i32, i64, f16, f32, f64, TDim] => |_, _| {
Ok(())
};
q: [i8, u8, i32] => |x: f32| x);
Copy link
Author

Choose a reason for hiding this comment

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

Not really sure what this q part does, for now just copied from the other ops

Copy link
Collaborator

Choose a reason for hiding this comment

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

The q section is for dealing with quantized datum types by converting to f32.

@kali
Copy link
Collaborator

kali commented Feb 1, 2024

Thanks for your contribution.

But... tract has immutable tensor semantics, so it does not need a copy operator. I must say I fail to see why NNEF needs one to be honest. I assume it's for some kind of aesthetic completion.

So unless I miss something, it should be mapped to... well nothing, or eventually to the operator Identity.

Am I missing something ?

@mmagician
Copy link
Author

You're right in that it's an identity operator- although it does carry the quantization information that's applied to the input & output.

@kali
Copy link
Collaborator

kali commented Feb 9, 2024

Mmm... So should we map it to a cast operator instead ?

I'm saying this knowing tract cast semantics are weak: half of them are conversions and the other half are reinterprets. This need sorting out. But in the meantime, we may be lucky and your model may work...

@mmagician
Copy link
Author

I'm happy to adapt the PR as you suggest. Will a cast preserve quantization then?

@kali
Copy link
Collaborator

kali commented Feb 9, 2024

Maybe I misunderstood. When you mentioned the quantization, I guessed that the copy operator was helping with converting from one conversion to another (as defined per a graph.quant file). Did I got this wrong ?

So if that really the case, you want an operator that will act as a conversion (like, actually recomputing stuff as the bytes representing the same values in the input and output quantization scheme will be different) and not a reinterpret cast (that would just switch the quantization parameter, not computing anything, not altering the bytes in the tensor).

I checked the code, and I think a cast operator between two quantization in tract will do a conversion. But as I said, this is a dark corner of tract at this stage, so we may have surprises.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants