search

bstatcomp / math

Stan Math Library
BSD 3-Clause "New" or "Revised" License
2 stars 0 forks source link

Feature/more rework adj jac apply #19

Closed bbbales2 closed 4 years ago

bbbales2 commented 4 years ago

I converted the last few functions that use adj_jac_apply to the new thing.

I'm gonna put a couple questions inline.

Edit: It was very easy. I think we should deprecate adj_jac_apply for the next release.

Also we could have called this tadj_jac_apply.

  1. Technically this operation is transpose(adjoint) * Jacobian, so that is a more precise name
  2. Tadej -> Tadj -> tadj -> tadj_jac_apply is a good bit of easter-egg history

It's a shame reverse_pass_callback is a good name.

bbbales2 commented 4 years ago

I'm happy with this code now. You can either merge it into your branch or we'll do it as a separate pull req. into Stan. Might be simpler to do the second.

SteveBronder commented 4 years ago

The current adj_jac PR is pretty small so I think it would be fine to merge into there. Though again because I goofed the merge earlier it may be better to wait for the adj_jac PR based on develop, then just squash this branch and cherry pick the squashed commit onto the adj_jac branch based off develop

Also I'm unironically okay with calling it tadj_jac_apply. I love the easter egg and if we are doing transpose(adjoint) * Jacobian it really is a more informative name