Skip to content

Add "edge" mode to mx.pad - #1309

Merged
barronalex merged 3 commits into
mainfrom
ab-pad-edge
Aug 6, 2024
Merged

barronalex merged 3 commits into
mainfrom
ab-pad-edge

Conversation

@barronalex

Copy link
Copy Markdown
Contributor

Resolves #1284

@barronalex
barronalex requested review from angeloskath and awni August 6, 2024 00:36
Comment thread mlx/ops.h Outdated
int destination,
StreamOrDevice s = {});

enum PaddingMode { Constant, Edge };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe use an enum class here instead of a enum

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On the other hand we tend to match types from Python API to C++ API (I'm not sure that's the best choice tbh), but for the sake of consistency maybe simpler to have it be a string at the ops.h level and scope the enum to the Primitive itself.

I think (?) it also makes it easier for code binding ops.h (i.e. MLX C) to not have to add the new enum type.

E.g. like indexing for meshgrid, ord for norm.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I changed it to just be a string.

@awni awni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great to me, thanks for adding that!

@barronalex
barronalex merged commit 635ccd9 into main Aug 6, 2024
@barronalex
barronalex deleted the ab-pad-edge branch August 6, 2024 18:23
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.

[Feature Request] mx.pad supports the "edge" padding mode.

2 participants