Replace MuladdMacro with manual application of muladd
#272
Conversation
devmotion
changed the title
muladdmuladd
YingboMa
left a comment
YingboMa
left a comment
There was a problem hiding this comment.
It looks good to me. Just need to change broadcasts into ntuples.
| end | ||
| end | ||
| else | ||
| # Note: we don't want to do broadcasting with only 1 multiply (no `+`), |
There was a problem hiding this comment.
This comment is no longer true. Do you mind to delete it?
There was a problem hiding this comment.
In fact, it might be better to actually do what is stated in the comment and to not apply broadcasting in this case. Somehow I missed the comment.
There was a problem hiding this comment.
I switched back to the old non-broadcasting behaviour in this case.
|
Since Travis tests don't run anymore and Github Action only runs with the latest Julia version (ref: #267), I checked locally that tests pass with Julia 1.0 and there are no problems with broadcasting |
|
I have PRs open to get us to GHA, they are jsut a long way down my backlog :-( |
|
@devmotion can you give this a quick rebase so it gets the new CI and then we can merge? |
devmotion
force-pushed
the
muladdmacro
branch
from
fcd245d to
2cfd7e4
Compare
Codecov Report
@@ Coverage Diff @@
## master #272 +/- ##
==========================================
- Coverage 89.73% 87.79% -1.94%
==========================================
Files 13 13
Lines 458 418 -40
==========================================
- Hits 411 367 -44
- Misses 47 51 +4
Continue to review full report at Codecov.
|
|
I rebased the PR and updated |
|
CI failing Ubuntu for Julia 1.0 is because for some reason that allocates less (unlike Mac or Windows). It is a unexpected pass. |
5 participants
This PR removes the dependency on the MuladdMacro package and instead explicitly adds
muladdcalls to the constructed expression.The propagation expression is so "clean" with clearly define multiplications and additions that the
muladdcalls can easily be added manually instead of applying the@muladdmacro that takes care of many special cases such as subtraction etc.There is one catch to it: it seems currently the
@muladdmacro is applied to the full expression including the partials and therefore also rewrites the user-provided definitions whereas this manual construction only addsmuladdcalls to the products and sum of partials and input gradients.