Feature/decl assign params #1441
Conversation
|
@WardBrian this seems to be failing because of a failed formatting, but I ran the formatter locally and seems fine? Tried to update all my stuff from opam and the scripts but I'm still getting the same formatting. |
There was a problem hiding this comment.
The failure is in Debug_data_generation_tests.ml. Some of the randomly generated numbers are off, but only in the least significant bit. Floating-point numbers are notoriously finicky. Could be some difference in rounding mode or whatever?
Running locally, I see the same results as Jenkins. You can just revert Debug_data_generation_tests.ml, this PR shouldn't affect that file anyway.
Thanks, fixed! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1441 +/- ##
==========================================
- Coverage 89.62% 89.56% -0.06%
==========================================
Files 63 63
Lines 10548 10593 +45
==========================================
+ Hits 9454 9488 +34
- Misses 1094 1105 +11
|
|
So I absolutely need a thorough set of eyes on the optimizer code. I think I got the lazy code motion correct |
| @@ -458,7 +458,8 @@ let create_decl_with_assign decl_id declc decl_type initial_value transform | |||
| () } in | |||
| let decl = | |||
| Stmt. | |||
| { Fixed.pattern= Decl {decl_adtype; decl_id; decl_type; initialize= true} | |||
| { Fixed.pattern= | |||
| Decl {decl_adtype; decl_id; decl_type; initialize= Default} | |||
There was a problem hiding this comment.
I think this should use the logic on the next line which inspects the rhs_assignment to set Initialize
There was a problem hiding this comment.
Looking at this code it's not modifying the params. Making changes in places that do no modify params is out of the scope of this PR.
There was a problem hiding this comment.
Sounds good -- I think we would really want to fix #1295 before doing this -- probably by writing a generic-ish check_size function in Stan which we can call after the assignment is completed.
There was a problem hiding this comment.
Ok @SteveBronder, assuming you don't take issue with any of the changes I just pushed this should be good to merge after Jenkins is done
Adds an
assignmentmember to theDeclrecord which allows for assignments made inline with the declaration to be a single line. For now this is only being done for parameters. Parameters are also given the auto type so that Eigen matrices and vectors can come in asEigen::Maptypes. This will stop parameters from being copied when used in later functions.Submission Checklist
Release notes
Make parameters
autoso that the parameter types for matrices can be Eigen mapsCopyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)