Stan code canonicalizer #390

nhuurre · 2019-11-19T12:30:29Z

Based on issue #262.

A new command line option that removes deprecated syntax from Stan code and pretty prints the result. No attempt is made to infer priors.

I still need to figure out what tests are required. The pretty printer seems to have nothing but a pretty.expected file in every subdirectory of test/integration/good. How are those generated?

Copyright holder: Niko Huurre

By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:

Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

seantalts · 2019-11-20T15:48:19Z

dune runtest runs all of those tests. They're all expect tests based on the dune files in each directory. We could add some tests for the canonicalizer in a similar way to the pretty printer, though probably not as many.

seantalts

This looks really good - was this your first ocaml?? We just needs some tests. You could make a new canonicalizer test folder and add a dune file that is really similar to any of the ones in good with stanzas for pretty printing (but with the argument changed from --pretty-print to whatever.

seantalts · 2019-11-20T16:00:54Z

src/frontend/Canonicalize.ml

+let deprecated_distributions = String.Table.create ()
+
+let () =
+  Hashtbl.add_multi deprecated_functions ~key:"multiply_log" ~data:"lmultiply" ;


Optional nit: we don't actually need Hashtbl with its mutability; we could just create an immutable Map here. I know we use this Hashtbl pattern elsewhere for the immutable stan math signatures - not sure if that's a pattern we want to keep using or not even for really large amounts of data.

Map would look something like:

let deprecated_distributions = String.Map.of_alist_multi [("multiply_log", "lmultiply") ; ("binomial_coefficient_log", "lchoose") ... ] @@ List.concat_map Middle.Stan_math_signatures.distributions ~f:(fun (fnkinds, name, _) -> List.map fnkinds ~f:(function | Lpdf -> (name ^ "_log", name ^ "_lpdf") ...

seantalts · 2019-11-20T16:06:18Z

src/frontend/Canonicalize.ml

+let rename_distribution name =
+  match Hashtbl.find deprecated_distributions name with
+  | None | Some [] -> name
+  | Some (rename :: _) -> rename


If we only use the first element of the value, why are we using a multi map?

Because it's my first Ocaml and I have no idea what I'm doing :P. The obvious alternative Hashtbl.add had a weird return type.

Hah, this is great for a first run! Would you mind changing it? You can use add_exn instead.

seantalts · 2019-11-20T16:08:20Z

src/frontend/Canonicalize.ml

+    | GetLP -> GetTarget
+    | FunApp (f, {name; id_loc}, e) ->
+        if is_distribution name then
+          CondDistApp


This changes the structure of the tree (replacing a FunApp with a CondDisApp). Is that because sometimes we have incorrectly labeled distributions as FunApps prior to this stage?

Deprecated density functions with _log suffix are real functions and are called as such. They're replaced by _lpdf suffix functions and those require the vertical bar syntax.

Woah. Stan3 says this code is incorrect

target += std_normal_lpdf(x);

and expects instead

target += std_normal_lpdf(x|);

but Stan2 can't parse that.

Hm, looks like the former should be okay. Let me make an issue for this.

seantalts · 2019-11-21T20:25:15Z

src/stanc/stanc.ml

@@ -83,6 +84,9 @@ let options =
    ; ( "--auto-format"
      , Arg.Set pretty_print_program
      , " Pretty prints the program to the console" )
+    ; ( "--canonize"


Hahaha. What about --print-canonical or something like that?

sure, sounds good.

nhuurre · 2019-11-21T21:39:30Z

Thanks for the review.

A question I'd like input on is whether to use typed or untyped AST. For now it's untyped because that's what the pretty printer expects by default but should be easy switch. Either way, there's a problem:

Stan2 incorrectly allows _lpdf suffix in tilde statements. This is checked for and throws an error in Semantic_check.ml. If this deprecated construct is to be fixed it must be done in untyped AST (or sematic check must be changed to allow it after all).
the floating point absolute value function abs is deprecated and should be replaced by fabs when the argument is real; abs is still used as the integer absolute value function. Fixing this requires knowing the type of the argument so can only be done with typed AST.

seantalts · 2019-11-22T00:49:11Z

An interesting conundrum! I think a good answer to this in the future might be that we split up type inference from type checking. That would make post-inference a really logical place for this phase. @enetsee has talked about this before but is pretty busy atm.

At the moment, could it make sense to have two phases in canonicalize, one pre and one post checking?

nhuurre · 2019-11-23T19:07:48Z

File "/root/.opam/4.07.0/lib/core_kernel/core_kernel.dune", line 1, characters 0-0:
Warning: .dune files are ignored since 2.0. Reinstall the library with dune
>= 2.0 to get rid of this warning and enable support for the subsystem this
library provides.

File "src/analysis_and_optimization/dune", line 4, characters 2-16:
4 |   (inline_tests)
      ^^^^^^^^^^^^^^
Error: No inline tests backend found.

Hmm. I don't think that's my fault. All tests pass on my computer.

Anyway, I think this is basically done. A few more thoughts:

User defined functions with _log suffix are also deprecated and should be renamed. Semantic_check.mli won't let me look at the global symbol table so I'd have to read them directly from the AST. I think that would work fine (because shadowing is not allowed). I'd rather wait for a more cooperative semantic checker, though. There's also the issues of what to do if both _log and the corresponding _lpdf are defined.

The last remaining deprecated feature is lkj_cov distribution. Stan2 says

Info: the lkj_cov_log() sampling distribution is deprecated.  It will be removed in Stan 3.
Code LKJ covariance in terms of an lkj_corr() distribution on a correlation matrix and independent lognormals on the scales.

Strangely, Stan3 does not emit a warning when compiling test/integration/good/lkj_cov_deprecation1.stan.
Anyway, rewriting lkj_cov seems basically impossible.

Another transform I considered is combining declare and define.

real x;
real y;
x = f(z);

could be changed to

real y;
real x = f(z);

Makes the code a bit more compact. Now that declarations are allowed anywhere it's easy to simply defer the declaration until the first assignment, if it is within the same basic block. This is safe assuming the code does not use uninitialized variables. (Moving stuff between basic blocks would require complex analysis that's not available in AST.)

seantalts · 2019-11-25T18:57:10Z

Hm, let me take a look at that failure - agreed it's probably not related to the code here.

seantalts · 2019-11-25T19:03:39Z

What do you need the global symbol table for exactly? It seems off the cuff like it could be better to make an isolated pass that works on an AST than to couple this to the semantic check pass, even if that saves some duplicated work.

Strangely, Stan3 does not emit a warning when compiling test/integration/good/lkj_cov_deprecation1.stan.

Seems like it should! Would you mind filing an issue?

I don't have a strong opinion that declare-and-define is always superior, but seems fine to me. If there's a revolt we can always remove it :) But right now the code can rely on uninitialized variables being initialized to NaN (which is a feature I hate and would love to change). We might be able to change that already, I think there's only a couple of MPI tests in CmdStan that rely on that.

We do want to move stuff between basic blocks eventually :) Definitely for optimization and possibly also for canonicalization, not sure.

nhuurre · 2019-11-25T23:13:13Z

Yeah, I don't know what I was thinking. The only problem with renaming user defined functions is that if there's a function or variable with the same name anywhere in the model it won't compile. But having something called foo_lpdf when you also have foo_log function is really really weird and the pretty printer will complain anyway if its output doesn't compile. So I guess there's no need to worry about it.

I haven't looked at the optimizer; I assume it handles many complex code transformations already. But it does all its work with the MIR so the results cannot be printed as Stan code.

There's an open issue for missing deprecation warnings. #165
(Maybe also useful for checking the completeness of the canonicalizer.)

I think I withdraw the declare-and-define thing. Cleaning up deprecated code is enough for one PR.

seantalts · 2019-12-02T17:29:02Z

Thanks! Will get to this today or Wednesday I hope - uncharacteristically busy the past week and the next week or so.

seantalts · 2019-12-03T21:12:59Z

src/frontend/Canonicalize.ml

+                   String.drop_suffix name 4 ^ "_lpdf"
+                 else String.drop_suffix name 4 ^ "_lpmf"
+               in
+               String.Table.add deprecated_userdefined ~key:name ~data:newname


Do we actually want to ignore duplicates or do we want to do something with them? I'll merge for now but just food for thought going forwards.

Duplicates come from forward declarations (which is needed if the function is recursive). The signatures given in function declaration and definition must be identical and no other function may have the same name.

seantalts · 2019-12-03T21:13:30Z

This looks great, congrats on your first OCaml code and PR to the repo :) I'll add you to the contributors list.

add rudimentary code canonicalizer

3eea5fe

seantalts requested changes Nov 21, 2019

View reviewed changes

nhuurre added 4 commits November 22, 2019 17:29

prefer String.Map over Hashtbl

958171c

canonicalize after typechecking

4499b03

more deprecations and fix de-parenthesizing bug

249b51d

one more corner case and a couple of tests

ccc296a

fix recursion

53b3b4d

rename deprecated userdef functions

08df3cc

seantalts mentioned this pull request Dec 2, 2019

target += distribution syntax for single argument incorrect #393

Closed

userdef _cdf_log is also deprecated

736b137

nhuurre requested a review from seantalts December 2, 2019 13:25

seantalts reviewed Dec 3, 2019

View reviewed changes

seantalts approved these changes Dec 3, 2019

View reviewed changes

seantalts merged commit f9b98d7 into stan-dev:master Dec 3, 2019

nhuurre deleted the canonicalizer branch December 4, 2019 07:52

nhuurre mentioned this pull request Sep 15, 2020

Deprecation warnings are missing for functions #600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stan code canonicalizer #390

Stan code canonicalizer #390

nhuurre commented Nov 19, 2019 •

edited

Loading

seantalts commented Nov 20, 2019

seantalts left a comment

seantalts Nov 20, 2019

seantalts Nov 20, 2019

nhuurre Nov 21, 2019

seantalts Dec 2, 2019

seantalts Nov 20, 2019

nhuurre Nov 21, 2019

nhuurre Nov 21, 2019

seantalts Dec 2, 2019

seantalts Nov 21, 2019

nhuurre Nov 21, 2019

nhuurre commented Nov 21, 2019

seantalts commented Nov 22, 2019

nhuurre commented Nov 23, 2019

seantalts commented Nov 25, 2019

seantalts commented Nov 25, 2019

nhuurre commented Nov 25, 2019

seantalts commented Dec 2, 2019

seantalts Dec 3, 2019

nhuurre Dec 4, 2019

seantalts commented Dec 3, 2019

Stan code canonicalizer #390

Stan code canonicalizer #390

Conversation

nhuurre commented Nov 19, 2019 • edited Loading

seantalts commented Nov 20, 2019

seantalts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuurre commented Nov 21, 2019

seantalts commented Nov 22, 2019

nhuurre commented Nov 23, 2019

seantalts commented Nov 25, 2019

seantalts commented Nov 25, 2019

nhuurre commented Nov 25, 2019

seantalts commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantalts commented Dec 3, 2019

nhuurre commented Nov 19, 2019 •

edited

Loading