unify compile-fn and compile-state-fn; primitive mode for compiled fns #115
Conversation
Redesign compiler as a pipeline of transformations that works by having each step add or update keys in a map representing the current state of the compilation, avoiding the need for bookkeeping parameters.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
- Coverage 86.45% 86.43% -0.03%
==========================================
Files 99 99
Lines 15306 15353 +47
Branches 785 798 +13
==========================================
+ Hits 13233 13270 +37
+ Misses 1288 1285 -3
- Partials 785 798 +13
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
- redo cache to include relevant information in cache key
- this makes the `*` and non-`*` versions of compile unnecessary
- You can supply the option `{:cache false}` if you really want to
- non-generic-params compilations are excluded from caching
- primitive calling convention now "works", but not yet wired into ODE
|
@sritchie once asked "why not just use Clojure as the representation of code and translate everything to JS at the last minute" and this PR is proof that that was a good idea. |
|
Amazing, I will review this tomorrow and kick the tires! |
src/emmy/numerical/ode.cljc
Outdated
| @@ -173,7 +173,7 @@ | |||
|
|
|||
|
|
|||
|
|
|||
| (defn integration-opts | |||
| (defn- integration-opts | |||
There was a problem hiding this comment.
If you don't mind, can you make this (defn ^:no-doc ...) instead of private? I think I need this from some of the simulation code I was writing, though that may not be true anymore. That metadata will keep the var out of public docs, but not actually keep the var private.
Your call, I can always use #' to get the var.
There was a problem hiding this comment.
I do not know why I changed this: maybe a prompt from the linter but all seems well with :no-doc
| (let [f' (c/compile-state-fn state-derivative derivative-args initial-state)] | ||
| (fn [y] | ||
| (f' y (or derivative-args [])))) | ||
| (c/compile-state-fn state-derivative derivative-args initial-state |
There was a problem hiding this comment.
one mode I would like to see, since it matters for the animation code, is "I am already giving you something compiled with a primitive calling convention, please just use it as is."
This comes up because we send source over the wire to Clerk in the browser, and it felt right to have Reagent trigger a final eval step every time new source shows up, but then pass it in directly to the ODE solver powering the animation.
There was a problem hiding this comment.
That would also let us delete the timing code below, since the user could compile the function themselves and then call an instrumented combinator if they wanted to use it, and hang on to their own stopwatch reference.
| equations (fn [_ y out] | ||
| ;; TODO: should we consider allowing an option to add a dummy |
There was a problem hiding this comment.
Removing this timer code made a significant difference in performance for the animations. I would recommend that we remove it here...
See my comment above for a possible workaround. We could also take an :instrument? flag, of course. Thoughts?
There was a problem hiding this comment.
The double-decker state derivative is annoying to deal with in this case since (in the SICM-book-compatible cases) we want to instrument the inner function that is returned from the application to derivative-args. So I'm going to take it out for now--as you say the customer can arrange for it themselves
src/emmy/numerical/ode.cljc
Outdated
| (us/stop evaluation-time)) | ||
| (derivative-fn y out primitive-params) | ||
| (us/stop evaluation-time) | ||
| (swap! evaluation-count inc)) | ||
|
|
||
| integrator (stream-integrator equations 0 flat-initial-state opts)] | ||
| {:integrator integrator |
There was a problem hiding this comment.
if we didn't instrument by default, and forced the customer to do it, we could just return the integrator here, which feels cleaner. Maybe that's a clue that this is the right move. What do you think?
There was a problem hiding this comment.
I think this conclusion is correct.
| ~body)))) | ||
| - `:body`: a function body making use of any symbol in argv above" | ||
| [{:keys [argv body]}] | ||
| (w/postwalk-replace sym->resolved-form `(fn [~@argv] ~body))) |
There was a problem hiding this comment.
sym->resolved-form smells like the next keyword option, probably for another PR. Some other dictionaries of math that I would use:
- https://github.com/generateme/fastmath/blob/master/src/fastmath/core.clj
- the
emmy.genericfunctions, if, for example, I wanted to send a function across the wire and then compile it back to generic - this is how we might think about compiling, say, matrix-valued functions (that wouldn't work with the primitive calling convention, but it would be great with the other options!)
| its code object argument in a form suitable for application of the Function | ||
| constructor." | ||
| [{:keys [argv body]}] | ||
| (let [argv (mapv commafy-arglist argv) |
There was a problem hiding this comment.
nice, so this gets all of the bindings up at the top.
src/emmy/expression/compile.cljc
Outdated
| current runtime environment." | ||
| [code] | ||
| #?(:clj (eval (compile->clj code)) | ||
| :cljs (comp js->clj (apply js/Function (compile->js code))))) |
There was a problem hiding this comment.
do we still need this js->clj in every case? Not in the primitive calling convention case, right?
| compiled)))))) | ||
| (compile-fn f n {})) | ||
| ([f n opts] | ||
| (let [argv (into [] (repeatedly n #(gensym 'x)))] |
There was a problem hiding this comment.
If we make parameters a keyword option, do you think we could merge compile-fn and compile-state-fn into a single thing? compile-state-fn would be a compile-fn call with parameters supplied, otherwise the function's treated as one level deep?
There was a problem hiding this comment.
Currently I delegated it the other way, but it could be reversed (and I agree that direction of delegation makes more sense conceptually) but "state function nature" has a large footprint. Let me think a little more about that
|
small request after staring at the animation code... could we add a |
|
on the matter of Another way to handle it would be to wrap the compiled function with a dispatcher. Maybe that is the way to handle instrumentation as well. You could pass an option, perhaps called With that idea, you could instrument and emit or even pass the composition of the two combinators to |
|
@littleredcomputer I grew deeply confused when I read your comment... why was emit ever working for me when I wired it in?? After a desperate minute I remembered that my |
- avoid js->clj in primitive case - remove timing instrumentation in ODE fns - `integration-opts` is now `make-integrator*` & is simpler
|
Understood. Then let's have both The ODE code will select |
|
That sounds great, and I think we now have all of my animation use cases covered. Just in time, since it looks like there will be at least one other talk that's using the animation libraries to go over some physics and math. |
This is on hold because it's not obvious how to paste up a call to an arbitrary function inside our "compiler". One way to do it would just be to |
From the CHANGELOG:
unify compile-fn and compile-state-fn; primitive mode for compiled fns #115:
This PR introduces significant upgrades to the functions in
emmy.expression.compile.compile-state-fnandcompile-fnnow share thesame code. Expect some more shifts here as we work on animations.
Specifically, this update:
Removes timers from the code in
emmy.numerical.ode. If you want timingdata you can generate an derivative yourself that performs timing. Including
this by default introduced a significant performance cost.
Renames
emmy.numerical.ode/integration-optstomake-integrator*; it nowreturns only a call to
stream-integratorinstead of the old map of thisresult and timers.
In
emmy.expression.compile:compile-state-fn*andcompile-fn*are now removed, in favor of thenon-starred versions with option
:cache? falsesupplied.compile-fnnow callscompile-state-fnwithparamsset tofalseandan
:arityoption supplied, vs using its own mostly-duplicatedimplementation.
:flatten?option removed in favor of the more granular:calling-convention. This option supports values of:structure,:flatand:primitive. See the docstring for details.:modesupports:js,:source,:clj,:nativeor:sci.