Skip to content

Callbacks phase 1 - #299

Open
JimClarke5 wants to merge 22 commits into
tensorflow:masterfrom
JimClarke5:Callbacks_Phase_1
Open

Callbacks phase 1#299
JimClarke5 wants to merge 22 commits into
tensorflow:masterfrom
JimClarke5:Callbacks_Phase_1

Conversation

@JimClarke5

@JimClarke5JimClarke5 commented Apr 25, 2021

Copy link
Copy Markdown
Contributor

This is the first Phase of Callbacks and is just a minimal set to start work on Model.

There is a chicken and egg relationship with Callbacks and Model, so this PR just addresses the first part with integration with Model postponed. Only enough Callbacks to get started with Model are provided for now.

Once model is done, then a 2nd phase PR with full integration with Model will be created.

@JimClarke5
Merge pull request#3from tensorflow/master
c57a2e7
Sync with master tensorflow on upstream
@JimClarke5
Merge pull request#4from tensorflow/master
09fc07e
Merge main branch to local branch
@JimClarke5
Merge pull request#5from tensorflow/master
a99dcb4
Update after losses merge
@JimClarke5
Merge pull request#6from tensorflow/master
ba294ea
Fix Javadoc errors (tensorflow#152)
@JimClarke5
Merge pull request#7from tensorflow/master
04f419a
pull type def
@JimClarke5
Merge pull request#8from tensorflow/master
02e7ebf
merge
@JimClarke5
Merge pull request#9from tensorflow/master
e0c9ed8
Metrics Phase 1 (tensorflow#180)
@JimClarke5
Merge pull request#10from tensorflow/master
5b0374b
Pull latest tensorflow master
@JimClarke5
Merge pull request#11from tensorflow/master
e038bbd
Merge with latest
@JimClarke5
Merge pull request#13from tensorflow/master
def3051
Resync with origin/master
@JimClarke5
Merge pull request#15from tensorflow/master
11748ae
Sync with tensorflow/java master
@JimClarke5
Merge pull request#16from tensorflow/master
a9412ea
Sync
@JimClarke5
Merge pull request#17from tensorflow/master
2ff8dfe
Sync with Metrics Phase 2
@JimClarke5
Initial checkin
df56f1d
@JimClarke5
Merge pull request#18from tensorflow/master
ee5e38a
Sync with master
@JimClarke5
Merge pull request#19from tensorflow/master
26394d6
Sync with Regularizers
@JimClarke5
Initial checkin
9dcddcd
@JimClarke5
Merge remote-tracking branch 'origin/Callbacks_Phase_1' into Callback…
ab2e304
…s_Phase_1
Craigacp
Craigacp requested changes May 6, 2021
View reviewed changes
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/CSVLogger.java
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/CSVLogger.java Outdated
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/CSVLogger.java Outdated
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/Callback.java Outdated

/** Creates a Callback */
protected Callback() {
this(null);

@CraigacpCraigacpMay 6, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would it be better to use Collections.emptyMap() here rather than null? Or is it mutated?

@JimClarke5JimClarke5May 6, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So far none of the subclasses write to it, so we could use emptyMap().
I don't think it is a good idea to leave it null. Also make it final in Callback.

Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/CSVLogger.java
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/util/ProgressBar.java
Comment threadtensorflow-framework/src/main/java/org/tensorflow/framework/callbacks/util/ProgressBar.java Outdated
Comment threadtensorflow-framework/src/test/java/org/tensorflow/framework/callbacks/CSVLoggerTest.java Outdated
Comment threadtensorflow-framework/src/test/java/org/tensorflow/framework/callbacks/CSVLoggerTest.java Outdated
Comment threadtensorflow-framework/src/test/java/org/tensorflow/framework/callbacks/LambdaCallbackTest.java Outdated
}

@Test
void onEpochEnd() {}

@CraigacpCraigacpMay 6, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These tests are empty?

@JimClarke5JimClarke5May 6, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will fix

@Craigacp

Craigacp commented May 6, 2021

Copy link
Copy Markdown
Collaborator

Mostly small stuff and some tidying in the docs. Otherwise it looks pretty good.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CraigacpCraigacpCraigacp requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JimClarke5@Craigacp