-
Notifications
You must be signed in to change notification settings - Fork 8
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
implement reverse #116
base: main
Are you sure you want to change the base?
implement reverse #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, then LGTM Bruce.
src/reverse.js
Outdated
*/ | ||
export function reverse(input, {axes}) { | ||
// reuse validateReduceParams to check parameters of reverse | ||
validateReduceParams(input, {axes}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If reverse
had been added before reduce
, would reduce
call validateReverseParams
? Renaming validateReduceParams
to a more generic validateAxes
would make sense.
52307c8
to
b09c509
Compare
src/lib/validate-input.js
Outdated
@@ -471,7 +471,10 @@ export function validatePool2dParams(input, _, {roundingType = 'floor'}) { | |||
} | |||
} | |||
|
|||
export function validateReduceParams(input, {axes}) { | |||
export function validateAxes(input, {axes}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the renaming, should it just take axes
argument rather than an object containing axes
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR is to fixe #110.
@fdwr @huningxin @shiyi9801 PTAL, thanks!