Closed inexorabletash closed 8 months ago
One thing that wasn't discussed in #572 in detail is: what to do with MLActivation
-vending methods? This PR currently simplifies the "create an MLActivation" steps so that they don't throw, on the assumption that the caller will validate. But none of the invocations do any validation (except clamp()
), and they all include "If that throws an error, re-throw the error." (except elu()
)
I think the right answer is: validate in the caller (e.g. the actual method), just like MLOperand
-vending methods. So drop the "re-throw" steps (like elu()
), and we can add validation steps if needed (like clamp()
). But confirmation would be appreciated.
Another slight tweak we might want to make to this - there are multiple styles used for declaring the operator. Here are all the distinct examples:
I'm not sure it really matters, but the "gru" style stands out as the odd duck. That said, there's something to be said for quoting the name, e.g. this reads strangely: "Let operator be an operator for the where operation..."
Spelling is also all over the place, e.g. "Leaky RELU" vs. "LSTM cell" vs. "Gather" vs. "instance normalization" vs. "batchNormalization".
there are multiple styles used for declaring the operator
That all is probably due to my sloppiness while mass-defining those algorithms at different times and fatigue levels :). Let's choose one and apply it consistently.
Thanks @inexorabletash ! I am not feeling well today. I'll catch up ASAP.
Okay, sounds like this is ready for a merge - @fdwr maybe one last look?
Okay, sounds like this is ready for a merge - @fdwr maybe one last look?
@inexorabletash : 🔎👀 ETA 16:10... (Oxford comma for the clarity win 👍)
Great work everyone!
I believe we were just able to squeeze these changes into the CR Snapshot release train before it departs. I expect the programming model section to be read by people outside this WG, so improvements there were timely.
As discussed in #572:
Not covered in this change:
this
For #549 and #572.
Preview | Diff