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 decomposition for ONNXSoftmaxCrossEntropyLossOp #2968

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

srcarroll
Copy link
Contributor

No description provided.

Signed-off-by: Sam <srcarroll314@gmail.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@srcarroll
Copy link
Contributor Author

srcarroll commented Oct 7, 2024

i'm not really familiar with them so will have to look into it, but should i add to test/backend/inference_backend.py or do there already exists relevant tests for this op (i only see shape inference tests)? would such a test belong somewhere else?

@srcarroll
Copy link
Contributor Author

srcarroll commented Oct 7, 2024

also i'm not considering the ignore_index attribute here and will leave as a todo. but i don't know of a good way to match fail this case. any suggestions?

Edit: actually just realized it is an optional, not defaulted, attribute. so i know how to check. but if anyone has ideas on how to extend this to support it, i'm all ears. the simplest thing i can think of is to just subtract the slice at ignore_index from the result of ReduceSum.

Signed-off-by: Sam <srcarroll314@gmail.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

Signed-off-by: Sam <srcarroll314@gmail.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@srcarroll
Copy link
Contributor Author

srcarroll commented Oct 8, 2024

i think i misunderstood the mean reduction with weights case. looking into it now (my recent "fix" commit is wrong)

@srcarroll
Copy link
Contributor Author

i think i misunderstood the mean reduction with weights case. looking into it now (my recent "fix" commit is wrong)

so i was just summing over the original weights tensor, but i should have been summing over W[n][d1][d2]...[dk] = weights[labels[i][d1][d2]...[dk]]. The simplest way i can think of producing W is doing onnx.Einsum(one_hot_labels, weights) {equation "ij...,j->i...} (or just a matmul if input is 2D). Any objection to this or any other suggestions? It probably wouldn't be very performant as one_hot_labels is a sparse matrix, but other solutions i can think of involve nasty indexing that i would like to avoid unless insisted upon.

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

Signed-off-by: Sam <srcarroll314@gmail.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@srcarroll
Copy link
Contributor Author

@jenkins-droid test this please

@AlexandreEichenberger you might want to cancel and rerun since i just pushed. also is there a rule of thumb for running tests? since this is still in draft i might push commits that shouldn't be tested yet. maybe i should just not have this drafted yet? sorry if i'm not following some etiquette for this

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

1 similar comment
@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

@srcarroll
Copy link
Contributor Author

@AlexandreEichenberger should i undraft to get some feedback on the testing i asked about? I figured i should get feedback first but i don't know of a good person to ping for this.

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.

3 participants