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

Add CallbackConstructorRegistry and semantics of ModelConfigFig.verbose #322

Merged
merged 7 commits into from
Sep 12, 2018

Conversation

caisq
Copy link
Contributor

@caisq caisq commented Sep 11, 2018

  • Add tfl.CallbackConstructorRegistry. Later the registry will be used in tfjs-node to register the ProgbarLogger.
  • Implement semantics of ModelFitConfig.verbose.

Towards: tensorflow/tfjs#631

This change is Reviewable

@caisq caisq changed the title [WIP] Add CallbackConstructorRegistry Add CallbackConstructorRegistry and semantics of ModelConfigFig.verbose Sep 11, 2018
Copy link

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @nsthorat, @davidsoergel, and @nkreeger)


src/base_callbacks.ts, line 610 at r1 (raw file):

      constructors.forEach(ctor => {
        if (ctor === callbackConstructor) {
          throw new ValueError('Duplicate callback constructor.');

print the name?


src/base_callbacks_test.ts, line 249 at r1 (raw file):

  beforeEach(() => {
    // tslint:disable-next-line:no-any
    (CallbackConstructorRegistry as any).clear();

do you need this cast?


src/base_callbacks_test.ts, line 331 at r1 (raw file):

  beforeEach(() => {
    // tslint:disable-next-line:no-any
    (CallbackConstructorRegistry as any).clear();

do you need this cast?

Copy link
Contributor Author

@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.

Thanks for the review!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq, @davidsoergel, and @nkreeger)


src/base_callbacks.ts, line 610 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

print the name?

There is no reliably usable name for the constructor, as it may get minimized.


src/base_callbacks_test.ts, line 249 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

do you need this cast?

Yes. The clear method is a protected method. I don't want to make it public because there is no need for the user to clear the registered callbacks. It is only for testing.


src/base_callbacks_test.ts, line 331 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

do you need this cast?

Same as above.

@caisq caisq merged commit 25c294e into tensorflow:master Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants