Skip to content

Update generated transform_inits_impl to directly accept a var_context.#1305

Merged
WardBrian merged 9 commits into
masterfrom
fix/deserializer-read-containers
Apr 6, 2023
Merged

Update generated transform_inits_impl to directly accept a var_context.#1305
WardBrian merged 9 commits into
masterfrom
fix/deserializer-read-containers

Conversation

@WardBrian
Copy link
Copy Markdown
Member

Submission Checklist

  • Run unit tests
  • Documentation
    • OR, no user-facing changes were made

Release notes

Closes #1304.

This PR does a couple things:

  1. Re-using the code gen we already have for the constructor, transform_inits_impl now takes in a var_context directly. This is what fixes [BUG] transform_inits code for complex numbers is incorrect. #1304, and also will help out with Add tuples to the language #1100. The previous behavior of flattening it down to a flat vector was hacky, and for complex numbers or tuples, wrong.

  2. Adds a new function called unconstrain_array which looks much more like the former transform_inits. This is the twin of write_array and can be eventually exposed/used in place of a lot of the places we're creating a array_var_context. The idea for a signature like this dates back to when the deserializer was added: Add deserializer class for reading constraints easier stan#3013 (comment). I added this here to keep the existing code for transform_inits_impl around rather than deleting it and re-coding it later, but it will currently be unused.

The second item was actually my original goal, but while working on it I uncovered #1304. The first nicely kills two birds with one stone, fixing that issue and helping out with something I had been stuck on in the tuples PR.

This was able to be done mostly by re-using code already in the compiler. Unfortunately, since the reading code is all generated in Transform_mir rather than lower down, the simplest solution was to add another (empty until the backend) item to the MIR.

Copyright 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)

Comment thread src/stan_math_backend/Transform_Mir.ml Outdated
Comment thread src/middle/Internal_fun.ml Outdated
Copy link
Copy Markdown
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, much cleaner! Code and gen'd c++ look good

Comment thread src/middle/Program.ml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] transform_inits code for complex numbers is incorrect.

3 participants