ngraph-tf
ngraph-tf copied to clipboard
NervanaSystems
Sarkars/builder
Please take a look at the builder refactoring change.
- Switching from old to new builder. The killswitch is a primeval C style macro in encapsulate_op.cc. Maybe there is a better/preferable method. But I used it instead of if(flag) because, when we enable the newbuilder at a later stage, we can just delete the stuff in the #if-#else en masse and not have to modify any more code. Currently, the old builder is in use, so nothing should fall apart.
#define BUILDER1 // (un)comment this line to (de)activate old builder
#ifdef BUILDER1 newbuilder #else oldbuilder #endif
-
I will continue adding more TranslateOps functions in the meanwhile
-
Since its a big change, I am sure there are style issues, questionable design/syntax choices, outright unoptimised chunks and sneaky leaks. Your feedback is very important and appreciated.
I think most of Adam's review comments are addressed or incorporated. The two things remaining are:
- T& vs T* for return. TF style prefers T* for return/changeable variables. references are preferred with consts. But in out code, we use TranslateGraph that uses non-const T&.
- Adding MakeNGraphNode as suggested by Adam in Slack. Should we do this in a new PR?
Any other thing that I missed in the above summary?
Comments below.
From: Sayantan Sarkar [email protected] Reply-To: NervanaSystems/ngraph-tf [email protected] Date: Monday, September 3, 2018 at 1:05 PM To: NervanaSystems/ngraph-tf [email protected] Cc: "Chakraborty, Avijit" [email protected], Mention [email protected] Subject: Re: [NervanaSystems/ngraph-tf] Sarkars/builder (#176)
I think most of Adam's review comments are addressed or incorporated. The two things remaining are:
-
T& vs T* for return. TF style prefers T* for return/changeable variables. references are preferred with consts. But in out code, we use TranslateGraph that uses non-const T&. I prefer T* for return as it’s explicit from the function signature. I used to believe that T& is safer but … but sure what’s the best practice these days.
-
Adding MakeNGraphNode as suggested by Adam in Slack. Should we do this in a new PR? I forgot which slack message. Could you please tag me in that one? I will review and let me know (Tomorrow -as I need to drive to LA now)
Any other thing that I missed in the above summary?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/NervanaSystems/ngraph-tf/pull/176#issuecomment-418185911, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AdGBtSXx2vn4E4PLZn4NPqErQAsbJ7Zzks5uXYtigaJpZM4WSXgC.
