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.

Sign up for GitHub

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

Jump to bottom

Add argminmax operator #778

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

Add argminmax operator #778

wants to merge 1 commit into from
+440 −3

Conversation

tmartin-gh
Copy link
Collaborator

@tmartin-gh tmartin-gh commented Oct 22, 2024 •
edited
Loading

TODO:

  1. Add unit tests
  2. Better names for 2output and 4output arg reduction

@cliffburdick
Copy link
Collaborator

/build

cliffburdick
cliffburdick reviewed Oct 22, 2024
View reviewed changes
include/matx/operators/argminmax.h

static __MATX_INLINE__ constexpr __MATX_HOST__ __MATX_DEVICE__ int32_t Rank()
{
return matxNoRank;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this have a rank of all the outputs?

cliffburdick
cliffburdick reviewed Oct 22, 2024
View reviewed changes
include/matx/transforms/cub.h
@@ -76,6 +76,7 @@ typedef enum {
CUB_OP_SELECT_IDX,
CUB_OP_UNIQUE,
CUB_OP_ARG_REDUCE,
CUB_OP_DUAL_ARG_REDUCE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quad?

cliffburdick
cliffburdick reviewed Oct 22, 2024
View reviewed changes
include/matx/transforms/reduce.h
@@ -2234,6 +2234,52 @@ void __MATX_INLINE__ argmin_impl(OutType dest, TensorIndexType &idest, const InT
#endif
}

/**
* Compute min and max reduction of a tensor and returns value + index
Copy link
Collaborator

Choose a reason for hiding this comment

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

Of an operator

cliffburdick
cliffburdick requested changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@cliffburdick cliffburdick left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Needs unit tests and documentation

@cliffburdick
Copy link
Collaborator

@tylera-nvidia please look at this one and test the performance if you still have that data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cliffburdick cliffburdick cliffburdick requested changes

@tylera-nvidia tylera-nvidia Awaiting requested review from tylera-nvidia

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmartin-gh @cliffburdick