[BUGFIX] enable test_fc_subgraph.py::test_fc_eltwise #20393
Conversation
|
Hey @sfraczek , Thanks for submitting the PR
CI supported jobs: [centos-gpu, windows-cpu, website, sanity, edge, unix-cpu, clang, miscellaneous, centos-cpu, windows-gpu, unix-gpu] Note: |
mseth10
added
the
pr-awaiting-testing
sfraczek
changed the title
|
|
||
| def _init_weight(self, _, arr): | ||
| mx.np.random.normal(self.mean, self.sigma, arr.shape, dtype=arr.dtype, out=arr) | ||
| # import pdb |
| #avoid calculating square root of negative values | ||
| self.alg = alg | ||
|
|
||
| def forward(self, x): | ||
| if self.alg == 'square_root': |
There was a problem hiding this comment.
Please add comment why we need abs here
There was a problem hiding this comment.
Is there any hidden reason besides the fact that square root can take only 0 and positive numbers as argument? It there is not I believe the comment is unnecessary.
There was a problem hiding this comment.
Only that square_root returns nans for negative numbers which fail on assert equals because nan != nan
| #avoid calculating square root of negative values | ||
| self.alg = alg | ||
|
|
||
| def forward(self, x): | ||
| if self.alg == 'square_root': |
There was a problem hiding this comment.
Is there any hidden reason besides the fact that square root can take only 0 and positive numbers as argument? It there is not I believe the comment is unnecessary.
| matched_list_.push_back(&new_node); | ||
| status_ = kSuccess; | ||
| return true; | ||
| } | ||
| if (new_node.op() == Op::Get("abs")) { | ||
| if (new_node.op() == Op::Get("abs") || | ||
| new_node.op() == Op::Get("_npi_absolute")) { |
There was a problem hiding this comment.
If you change something I think it would be nice to put those checks in pairs e.g. if (new_node.op() == Op::Get("abs") || new_node.op() == Op::Get("_npi_absolute")) { or new_node.op() == Op::Get("exp") || new_node.op() == Op::Get("_npi_exp").
There was a problem hiding this comment.
Like this?
I prefer it as it is
There was a problem hiding this comment.
Yes, I was thinking about something similar to this. That was just a suggestion so if you do not like it, feel free to ignore it :)
There was a problem hiding this comment.
ok thank you it was worth a try :)
mseth10
added
pr-work-in-progress
fix numpy activation after fc
mseth10
added
pr-awaiting-review
|
@mxnet-bot run ci[unix-cpu] |
|
Jenkins CI successfully triggered : [unix-cpu] |
mseth10
added
pr-awaiting-testing
fix numpy activation after fc fuse into subgraph
Description
Enables test_fc_eltwise
Checklist
Essentials
Comments
Fixes second part of issue #20354
Abs of input for square_root operation was introduced because otherwise nan's were in output and asserts wouldn't accept nans on the same positions to be equal even if assert_almost_equal_with_err equal_nan=True