Skip to content

Generic cleanup Metrics and Losses #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 62 commits into from

Conversation

JimClarke5
Copy link
Contributor

This PR effects both losses and metrics.

This PR cleans up generic parameters to cut down on the number of unique parameters to at most 1 where possible.
For example:
public abstract class Metric<U extends TNumber, T extends TNumber> {
becomes
public abstract class Metric<T extends TNumber> {

and

@Override
  public <V extends TNumber> List<Op> updateStateList(Operand<U> values, Operand<V> sampleWeights) {

Changes to:

 public List<Op> updateStateList(
      Operand<? extends TNumber> values, Operand<? extends TNumber> sampleWeights) 

Sync with master tensorflow on upstream
Merge main  branch to local branch
Update after losses merge
Fix JavaDoc,
modify one line code block to include braces.
…nly live within a single instance of a Metric.
…e same type as the return or internal variables,
change hasValidNonscalarShape to canBroadcastNonscalarShapes
change hasValidNonscalarShape to canBroadcastNonscalarShapes
move the dynamic shapes and rank down to the dynamic section so they are created needlessly when static
Fix if statement to check for unknown size and unknown dimensions
renamed WeightBroadcastTest to AssertBroadcastableTest and added BroadcastWeightsTest
JimClarke5 and others added 19 commits February 1, 2021 13:14
…e same type as the return or internal variables,
change hasValidNonscalarShape to canBroadcastNonscalarShapes
change hasValidNonscalarShape to canBroadcastNonscalarShapes
move the dynamic shapes and rank down to the dynamic section so they are created needlessly when static
Fix if statement to check for unknown size and unknown dimensions
renamed WeightBroadcastTest to AssertBroadcastableTest and added BroadcastWeightsTest
* Successfully remove extra type params, but it broke javadoc generation

Signed-off-by: Ryan Nett <[email protected]>

* Generate covariant types

Signed-off-by: Ryan Nett <[email protected]>

* Do generation

Signed-off-by: Ryan Nett <[email protected]>

* Update help text.

Signed-off-by: Ryan Nett <[email protected]>

* Fixes

Signed-off-by: Ryan Nett <[email protected]>
….sparse.sparseToDense with the output of tf.sparse.denseToDenseSetOperation
@google-cla
Copy link

google-cla bot commented Feb 1, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@JimClarke5
Copy link
Contributor Author

@karllessard @Craigacp @rnett Could one of you clue me in what went wrong here. I rebased the project to the latest master, but it still seem to pick up a lot of the commits from the metrics1 branch, that were applied with yesterday's merge.

@rnett
Copy link
Contributor

rnett commented Feb 1, 2021

Only time I've seen that happen is when I didn't update my local master, or I had a commit in my branch (that wasn't in master) from before master's latest commit.

Can you cherry pick the commits you want to a new branch? That's how I've handled it.

@JimClarke5
Copy link
Contributor Author

I found this article which sort of explains how to do it. Seems overly complicated.
How to Fix Your Git Branches After a Rebase

@Craigacp
Copy link
Collaborator

Craigacp commented Feb 1, 2021

I second Ryan's suggestion of git cherry-pick. Make a new branch from master, run cherry pick giving it the commits that should be in this PR, then force push that branch over this one (or make a new pr from the new branch).

There are other ways to fix it, but it depends on what the exact state of your git repo is.

@JimClarke5
Copy link
Contributor Author

JimClarke5 commented Feb 1, 2021

I had master->metrics1->generic_cleanup

I pulled yesterday's merge to my master from the main TF repository.
Then did a rebase of generic_cleanup to my master.

Based on recreating generic_cleanup with cherry-pick,
should I fix this PR? I assume cancel/replace.

@Craigacp
Copy link
Collaborator

Craigacp commented Feb 1, 2021

A force push will fix it, but it'll require a bit of a renaming dance with the branches (i.e. cherry-pick to new branch, delete old branch, rename new branch to old branch, force push). It's up to you whichever you find easiest.

@JimClarke5 JimClarke5 closed this Feb 1, 2021
@JimClarke5
Copy link
Contributor Author

Rebuilt the branch to exclude commits already included in master.

@JimClarke5 JimClarke5 deleted the generic_cleanup branch February 1, 2021 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants