[PATCH] D89626: [ML] Add final reward logging facility.

[Yundi Qian via Phabricator via llvm-commits]

yundiqian accepted this revision.
yundiqian added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:142-145
+  template <typename T> void logFinalReward(T Value) {
+    assert(RawLogData.back().empty());
+    logReward(Value);
+  }
----------------
mtrofin wrote:
> yundiqian wrote:
> > It seems over-complicated to pass the flag FinalReward to writeRawTensorsAsFeatureLists function and treat the case separately. 
> > 
> > How about making the RawLogData ready-to-print (reward vector is <0, 0, ..., 0, reward>) so that we don't need to change writeRawTensorsAsFeatureLists function? basically the user is supposed to make sure the data in RawLogData is ready-to-print and writeRawTensorsAsFeatureLists only takes care of printing format.
> > 
> > We can either:
> > 1. logReward(0) in each step
> > 2. have a function in Logger called logFinalReward(T value) or overwriteFinalReward(T value), which overwrites the value in RawLogData.back()->back()
> > 
> > or:
> > 1. don't logReward(0) in each step
> > 2. have a function in Logger called logFinalReward(T value) that fills in 0 in RawLogData.back() except putting reward in the last, we can tell the length by looking at feature length already logged in RawLogData
> That would mean keeping around a potentially large array containing 0 (except the last value).
> 
> I would rather not add memory overhead if it can be avoided, and the extra consideration in code isn't that hard to grok.
Since it's for development mode, I guess it is debatable whether it worth adding code complexity to trade for better efficiency. imo it does not worth it, but I'm open to either options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89626/new/

https://reviews.llvm.org/D89626