Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Aug 13, 2017

  • Fix tanh overflow error where tanh(-100) was NaN.
  • Fix scope bug where scope's return result of type NDArray[] was not kept in the parent scope
  • Add debug mode for math, where it throws an error the first time a NaN entry is found.

This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat August 13, 2017 00:24
@nsthorat
Copy link
Contributor

:lgtm_strong:


Reviewed 3 of 4 files at r1.
Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.


src/math/math.ts, line 67 at r1 (raw file):

   * and checked for NaNs. This significantly impacts performance.
   */
  enableDebugMode() {

I wonder if we should just call this enableNaNDetection or something more than just "enableDebugMode", unless you can think of some other things debug mode will do. For any other xxx detection we could have a bit for that as well. WDYT?


src/math/math.ts, line 159 at r1 (raw file):

  }

  private checkForNaN(arr: NDArray): void {

can you add a unit test for this?


Comments from Reviewable

@nsthorat
Copy link
Contributor

Reviewed 1 of 4 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


src/math/math.ts, line 67 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I wonder if we should just call this enableNaNDetection or something more than just "enableDebugMode", unless you can think of some other things debug mode will do. For any other xxx detection we could have a bit for that as well. WDYT?

I can imagine later adding other things to debug mode and not having the user call several debug methods. For example, since we are already downloading all of the values to CPU, we might want to console log [min, 5%-tile, median, 95%-tile, max] for each tensor, log the operation of the last math call (obtained using console.trace()), log the execution time (when we add gpu callbacks in webgl 2.0) etc


src/math/math.ts, line 159 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you add a unit test for this?

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 2db9024 into master Aug 13, 2017
@dsmilkov dsmilkov deleted the logical branch August 13, 2017 21:10
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
… math (tensorflow#34)

* Fix overflow tanh overflow error, scope bug and add debug mode for math

* simplify debug mode api

* fix typo in trig_gpu_test

* Merge master into logical

* Merge master into logical

* add unit tests for math debug mode

* Merge branch 'logical' of https://github.com/PAIR-code/deeplearnjs into logical
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