-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
CenterCrop Layer #6875
CenterCrop Layer #6875
Conversation
* Center Crop Preprocessing Layer Co-authored-by: David Kim (@koyykdy) <[email protected]> Brian Zheng (@Brianzheng123) <[email protected]>
@mattsoulanille Please take a look whenever you're available. Thanks! |
Hi @mattsoulanille, Thanks again for meeting with us this morning! After reviewing the feedback on our last two layers, I am pretty confident we have some repeated mistakes in this one. @koyykdy and I will review this layer and ping you once we have cleaned those up to try and make things easier on your end, so don't worry about reviewing it for now. Thanks, Adam |
Thanks for letting me know, @AdamLang96. I'll wait for your ping. |
@mattsoulanille I think this is ready for review whenever you're available. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks good. I just have a few style changes. Thanks!
Co-authored-by: Matthew Soulanille <[email protected]>
Co-authored-by: Matthew Soulanille <[email protected]>
Merge branch 'centercrop' of https://github.com/CodeSmithDSMLProjects/tfjs into centercrop
Merge branch 'centercrop' of https://github.com/CodeSmithDSMLProjects/tfjs into centercrop
Merge branch 'centercrop' of https://github.com/CodeSmithDSMLProjects/tfjs into centercrop
Merge branch 'centercrop' of https://github.com/AdamLang96/tfjs into centercrop
* refactored centercrop * Update package.json * Update yarn.lock * started fixing layer * finished style changes * added reviewer recs
* refactored centercrop * Update package.json * Update yarn.lock * started fixing layer * finished style changes * added reviewer recs * added semicolon to pass linter
@mattsoulanille I updated the layer to reflect your changes. Let me know what you think. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Merge branch 'centercrop' of https://github.com/CodeSmithDSMLProjects/tfjs into centercrop
@ahmedsabie Please take a look at this PR. Thanks! |
|
||
return tidy(() => { | ||
let input: Tensor4D; | ||
let rank3 = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename to isRank3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahmedsabie done!
This is failing
|
@mattsoulanille whoops just fixed it |
Thanks! |
David Kim (@koyykdy) [email protected]
Brian Zheng (@Brianzheng123) [email protected]
This change is