Skip to content
This repository has been archived by the owner on Jul 15, 2022. It is now read-only.

fix histogram render when NaN/Infinity is present #42

Merged
merged 2 commits into from
Dec 19, 2018
Merged

Conversation

tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Dec 18, 2018

Closes tensorflow/tfjs#999

Thanks for reporting this @caisq


This change is Reviewable

@tafsiri tafsiri requested a review from caisq December 18, 2018 22:36
Copy link

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @tafsiri and @caisq)


src/render/histogram.ts, line 90 at r1 (raw file):

!isNaN(val) && isFinite(val

isFinite() returns false for NaN. So I think you can omit the isNaN() check here.

Also, does this merit a unit test somewhere?

Copy link
Contributor Author

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @caisq)


src/render/histogram.ts, line 90 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
!isNaN(val) && isFinite(val

isFinite() returns false for NaN. So I think you can omit the isNaN() check here.

Also, does this merit a unit test somewhere?

Thanks for the suggestion. The canvas rendering makes testing this a bit tricky as there was no other error produced. Would have to rethink the testing infra a bit.

@tafsiri tafsiri merged commit b1da1d3 into master Dec 19, 2018
@tafsiri tafsiri deleted the 999-histogram-fix branch December 19, 2018 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants