Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug_data_generation fixes #1128

Merged
merged 4 commits into from
Feb 22, 2022
Merged

Debug_data_generation fixes #1128

merged 4 commits into from
Feb 22, 2022

Conversation

nhuurre
Copy link
Collaborator

@nhuurre nhuurre commented Feb 19, 2022

After #1115 Debug_data_generation.ml lives in analysis_and_optimization instead of frontend and it should no longer use Frontend.Ast. This PR removes Ast from data generation.
Also, generation of matrices with non-scalar lower/upper bounds was broken; I fixed that too.

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Copyright and Licensing

Copyright holder: Niko Huurre

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)

@WardBrian WardBrian self-requested a review February 21, 2022 00:39
Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

A few comments. This looks great overall

src/middle/Expr.mli Outdated Show resolved Hide resolved
src/frontend/Ast_to_Mir.mli Outdated Show resolved Hide resolved
Comment on lines 216 to 218
( StanLib (transpose, FnPlain, _)
, [{pattern= FunApp (CompilerInternal FnMakeRowVec, l); _}] )
when String.equal transpose (Operator.to_string Transpose) ->
Copy link
Member

Choose a reason for hiding this comment

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

[Optional] - I think it is easier to read if the pattern is StanLib ("Transpose__", .... rather than a guard clause here, but it is relying on how Operators are currently translated which I suppose could change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I could go either way. CompilerInternal got this one right but I guess operators gotta be StanLib.

src/frontend/Ast_to_Mir.ml Outdated Show resolved Hide resolved
@WardBrian
Copy link
Member

@serban-nicusor-toptal any idea why the compilation CI didn’t run on this most recent set of changes?

@serban-nicusor-toptal
Copy link
Contributor

It only runs when there are changes to test/integration/good

@WardBrian
Copy link
Member

I thought we ran it on changes to test/integration/good and src/?

I suppose it would be difficult to make a breaking change without touching the output of the dune tests

@serban-nicusor-toptal
Copy link
Contributor

At some point we separated them I think to speed up CI times tho if needed we can always change it.

@WardBrian WardBrian merged commit afb355e into stan-dev:master Feb 22, 2022
@nhuurre nhuurre deleted the datagen-mir branch February 22, 2022 14:20
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.

3 participants