Skip to content

Refactor: Extract set_output Function from utils.h - #4467

Open
JasonHonKL wants to merge 2 commits into
ml-explore:mainfrom
JasonHonKL:refactor/set_ouput
Open

Refactor: Extract set_output Function from utils.h#4467
JasonHonKL wants to merge 2 commits into
ml-explore:mainfrom
JasonHonKL:refactor/set_ouput

Conversation

@JasonHonKL

@JasonHonKL JasonHonKL commented Sep 5, 2026

Copy link
Copy Markdown
Contributor
  • ☑️ I understand it is strictly prohibited to use AI to write PR description

This PR extracts set_output function from normalization.cpp and softmax.cpp.

In total there are 3 same function call but using lambda function currently. This refactor allows us to reuse and reduce the complexity from the code.

  • AI usage disclosure:

Umm I used AI to move the the block to utils.h and understand this function especially the following line haha and also use some it to help me do the formatting and running the testing (I'm lazy to type the command loll)

no_copy &= (s == 0 || s == x.shape().back() || x.shape(-2) == 1);

@simeetnayan81

Copy link
Copy Markdown

Rather than randomly fixing the tech-debts, it'd be better if we can create a issue and track what needs to be done there. We can create sub-tickets under that for a tech debt that needs attention

@JasonHonKL

JasonHonKL commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the recommendations. Usually I would create an issue and fix it however sometime I just randomly explore like this time lolll. So I don't really have a list of "tech-debt" task lolll

@zcbenz
zcbenz force-pushed the refactor/set_ouput branch from a4109bf to 70c0a1b Compare September 8, 2026 00:12

@zcbenz zcbenz 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.

While this does reduce duplicate code, it does not really make code more readable, specifically with the new relax_prev_stride arg it would take more time for a reader to understand what the code exactly does. Different kernels can have very slight different requirements on inputs and it is hard to extract common functions without harming readability.

For this case, it would make sense moving set_ouput to a common function within the normalization.cpp‎ file though.

@JasonHonKL

Copy link
Copy Markdown
Contributor Author

gonna do it tn thx for the comment.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants