Fix isnan usage in RTC #18984
Fix isnan usage in RTC #18984
Conversation
|
Hey @ptrendx , Thanks for submitting the PR
CI supported jobs: [clang, edge, centos-gpu, miscellaneous, sanity, website, windows-gpu, centos-cpu, unix-cpu, windows-cpu, unix-gpu] Note: |
sxjscience
left a comment
sxjscience
left a comment
There was a problem hiding this comment.
For me, I think it might be difficult to add test cases and it should good to merge for now (since we are in the dev mode). We can always add test cases later to cover these cases. Thus, I will approve it but feel free to express your concerns @marcoabreu .
|
I mean we discovered a clear case where it fails, so why not add a test? |
|
Currently it's triggered if we train the ELECTRA-small model in GluonNLP: https://github.com/dmlc/gluon-nlp/tree/master/scripts/pretraining, which is somehow complicated. |
|
Maybe @ptrendx can extract the relevant part and create a unit test out of it? |
|
Yes, I can and will add a test (sorry for not answering it earlier). |
|
I added testing of integer datatype to maximum and minimum functions - currently |
|
Thanks! |
|
@mxnet-bot run ci [centos-gpu, windows-cpu] |
|
Jenkins CI successfully triggered : [windows-cpu, centos-gpu] |
|
@marcoabreu Do you think this PR is good to go? |
4 participants
Description
Fixes problem reported in #18622, where the non-templated version of
isnanwas used inside min and max functions, causing failures for integer types.