Skip to content
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

Add shuffleCombo #4446

Merged
merged 25 commits into from
Jan 15, 2021
Merged

Add shuffleCombo #4446

merged 25 commits into from
Jan 15, 2021

Conversation

GantMan
Copy link
Contributor

@GantMan GantMan commented Dec 27, 2020

When shuffling an array of X and Y it's important that X still corresponds with Y.

This is a useful utility for shuffling training data that is in JavaScript form.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

When shuffling an array of X and Y it's important that X still corresponds with Y.

This is a useful utility for shuffling training data that is in JavaScript form.
@google-cla google-cla bot added the cla: yes label Dec 27, 2020
@GantMan
Copy link
Contributor Author

GantMan commented Dec 30, 2020

@pyu10055 - I used this to shuffle X and Y in the same way.

@GantMan
Copy link
Contributor Author

GantMan commented Dec 31, 2020

I'm having trouble seeing what is failing

@tafsiri
Copy link
Contributor

tafsiri commented Jan 5, 2021

@GantMan
Copy link
Contributor Author

GantMan commented Jan 5, 2021

No I cannot access pantheon bc I do not have a google.com email.

Also, the Details link lands me in Google Cloud Build, which shows this:
image

@tafsiri
Copy link
Contributor

tafsiri commented Jan 5, 2021

Wrt to the details link, it should give you access if you join one of the two mailing lists linked above (I can't remember if a google account is required but it might be). Are you already on one of those lists?

@GantMan
Copy link
Contributor Author

GantMan commented Jan 5, 2021

@tafsiri - you rock. I missed that note, and now I can see the build issue. I'll dig into it.

@GantMan
Copy link
Contributor Author

GantMan commented Jan 5, 2021

Thanks @tafsiri for getting me access to the logs. Sorry to bounce this back and forth so many times but I couldn't run the scripts on Windows, so I needed that access.

Should be good for eval now.

@tafsiri tafsiri requested a review from pyu10055 January 5, 2021 20:22
Copy link
Collaborator

@pyu10055 pyu10055 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 1 approvals obtained (waiting on @GantMan and @pyu10055)


tfjs-core/src/util_base.ts, line 72 at r4 (raw file):

// tslint:disable-next-line:no-any
                      array2: any[]|Uint32Array|Int32Array|Float32Array): void {
  console.assert(array.length === array2.length);

can you throw exception instead of using console.assert? TFJS support other platforms where console.assert might not be supported.

@GantMan
Copy link
Contributor Author

GantMan commented Jan 7, 2021

@pyu10055 - good call, all fixed.

Copy link
Contributor Author

@GantMan GantMan left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: 0 of 1 approvals obtained (waiting on @GantMan)

Copy link
Contributor Author

@GantMan GantMan 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 1 approvals obtained (waiting on @pyu10055)


tfjs-core/src/util_base.ts, line 72 at r4 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

can you throw exception instead of using console.assert? TFJS support other platforms where console.assert might not be supported.

Done.

@GantMan GantMan requested a review from pyu10055 January 11, 2021 18:23
@GantMan
Copy link
Contributor Author

GantMan commented Jan 13, 2021

@pyu10055 - anything else on this one?

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Thanks Gant, can you add a unit test for this change?

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

Copy link
Contributor Author

@GantMan GantMan left a comment

Choose a reason for hiding this comment

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

@pyu10055 - I noticed there was no test for util.shuffle either so I added a test for the existing shuffle and for my new shuffleCombo.

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

Copy link
Contributor Author

@GantMan GantMan left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

Copy link
Contributor Author

@GantMan GantMan left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

@GantMan GantMan requested a review from pyu10055 January 15, 2021 20:28
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks

@pyu10055 pyu10055 merged commit 608e61d into tensorflow:master Jan 15, 2021
@GantMan GantMan deleted the patch-6 branch January 15, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants