Glow: [New operator] Logit

Created on 16 Oct 2018  路  4Comments  路  Source: pytorch/glow

We need to support Logit operator from Caffe2. Spec can be found here:
https://caffe2.ai/docs/operators-catalogue.html#logit

It's Caffe2 implementation can be found here:
https://github.com/pytorch/pytorch/blob/master/caffe2/operators/logit_op.cc#L14
We'd like to follow it closely.

More detailed: need to add createLogit() method to our Function class, which does "immediate lowering". I.e. creates all necessary min, max, subtraction (1 - x), division, log nodes, and returns resulting sequence (syntactic tree). No changes to IR or Backends is required.

good first issue

Most helpful comment

Sounds great, thanks!

I don't believe I have assignment rights (as in https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/), but consider me assigned!

(Note to self:) Looks like (after implementing createLogit itself; "Graph.h" & "Graph.cpp"), adding support to "Caffe2ModelLoader.cpp", together with an associated test in "caffe2ImporterTest.cpp" (with a related .pbtxt file) are next.

Will be sending a PR when done! :-)

All 4 comments

Hi!

I can take a stab, if the issue is still unclaimed! :-)

One question, though. Context: In order to compute the clamped value of the input data (as in the Caffe2 specification), we need to subtract the (broadcasted) epsilon value (named eps, following Caffe2) from one(s) -- and later on subtract the input data from one(s), too. The question is: Is it okay to reuse the ones node as in the following (in particular: nodes minArg and SN)?

  // Initial stab. Comments welcome; can send PR pending the does-this-make-sense confirmation.

  // Broadcast eps so that it is the same size as the data.
  auto *BE = createBroadcast(name.str() + ".broadcast", eps, data.dims(),
                              /*axis=*/1);

  // Compute clamped x = max(min(x, 1 - eps), eps).
  // First, compute 1 - eps.
  auto *ones =
      createSplat(name.str() + ".ones", BE->getResult().getType(), 1.0f);
  auto *minArg = createSub(name.str() + ".sub", ones, BE);
  // Take the min of data and 1 - eps.
  auto *MinN = createMin(name.str() + ".min", data, minArg);
  // Take the max of the min and eps (thus obtaining the clamped x).
  auto *MaxN = createMax(name.str() + ".max", MinN, BE);

  // Compute the logit transform of clamped x,
  // log(numerator / denominator),
  // where numerator = clamped x = MaxN,
  // and denominator = 1 - clamped x = 1 - MaxN.

  // Compute denominator = 1 - clamped x.
  auto *SN = createSub(name.str() + ".sub", ones, MaxN);

  // Compute the quotient = numerator / denominator.
  auto *DN = createDiv(name.str() + ".div", MaxN, denominator);

  // Compute and return the logit transform (the final node).
  return createLog(name.str() + ".log", DN);

Hi @MattPD, I believe it's still up for grabs -- feel free to assign it to yourself!

RE: your question, it is totally fine (and actually preferable) to reuse ones. In fact, if you didn't reuse it then there's a decent chance (depending on the size of ones) that our Constant deduplication pass would merge the duplicate Splats anyway 馃槂

Sounds great, thanks!

I don't believe I have assignment rights (as in https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/), but consider me assigned!

(Note to self:) Looks like (after implementing createLogit itself; "Graph.h" & "Graph.cpp"), adding support to "Caffe2ModelLoader.cpp", together with an associated test in "caffe2ImporterTest.cpp" (with a related .pbtxt file) are next.

Will be sending a PR when done! :-)

@MattPD : thank you for taking this!

One comment: we'd prefer to add test to operatorsTest, not caffe2ImporterTest. The caffe2-related component of this task is trivial and not really worth testing.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

pjaaskel picture pjaaskel  路  4Comments

jackm321 picture jackm321  路  3Comments

dati91 picture dati91  路  3Comments

s-peryt picture s-peryt  路  3Comments

wayneshawn picture wayneshawn  路  3Comments