Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

use safe accumulation for norm #14240

Closed
wants to merge 1 commit into from
Closed

use safe accumulation for norm #14240

wants to merge 1 commit into from

Conversation

szha
Copy link
Member

@szha szha commented Feb 23, 2019

Description

use safe accumulation for norm. resolves #14126

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Code is well-documented:
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • use safe accumulation for norm

@szha szha changed the title use safe aggregation for norm use safe accumulation for norm Feb 23, 2019
@roywei
Copy link
Member

roywei commented Feb 23, 2019

@mxnet-label-bot add[Backend, Operator, pr-awaiting-review]

@marcoabreu marcoabreu added Backend Issues related to the backend of MXNet Operator pr-awaiting-review PR is waiting for code review labels Feb 23, 2019
@eric-haibin-lin eric-haibin-lin self-requested a review February 23, 2019 05:52
@ptrendx
Copy link
Member

ptrendx commented Feb 23, 2019

I did not see it immediately from the code - what is the type of the result? Norm should output fp32 as it's result since it is very easy to overflow otherwise. Mean on the other hand can be done with just fp32 accumulator without changing the output type.

@szha
Copy link
Member Author

szha commented Feb 23, 2019

I didn't change the output type yet. Looks like I should add a dtype option to norm too.

"floating point types not int8"; \
{ \
typedef int8_t DType; \
typedef int8_t AType; \
Copy link
Member

Choose a reason for hiding this comment

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

can we support acc in int types, too?

@szha szha added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Feb 28, 2019
@szha szha closed this Apr 1, 2019
@haojin2 haojin2 mentioned this pull request Apr 4, 2019
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backend Issues related to the backend of MXNet Operator pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safe accumulation type for nd.norm
5 participants