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

Fix tests for optimizations #1183

Merged
merged 3 commits into from
May 3, 2022
Merged

Fix tests for optimizations #1183

merged 3 commits into from
May 3, 2022

Conversation

rok-cesnovar
Copy link
Member

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

@WardBrian can we add a "simple" optimization bug to stress test this? Something that will produce the wrong results.

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)

@WardBrian
Copy link
Member

I've introduced a bug which swaps the order of arguments to fma in the partial evaluator. If it still passes tests that is probably because none of the tests we're running feature a float multiply add. We next need to address stan-dev/performance-tests-cmdstan#38, e.g. we need more tests in this set that get optimized.

@rok-cesnovar
Copy link
Member Author

rok-cesnovar commented May 3, 2022

Thanks!

We next need to address stan-dev/performance-tests-cmdstan#38, e.g. we need more tests in this set that get optimized.

Agreed. Where do we want to put these? In the perf-cmdstan repo or stanc3?

We can then do the same for stan-dev/performance-tests-cmdstan#30

@WardBrian
Copy link
Member

It may be arbitrary, but to me I think anything which is testing that the model compiles (e.g., what you're proposing in stan-dev/performance-tests-cmdstan#30) can live here in stanc3, but anything which we want to actually run should be in performance-tests-cmdstan. Sound reasonable?

@rok-cesnovar
Copy link
Member Author

Yeah, good call.

@WardBrian
Copy link
Member

We already have https://github.com/stan-dev/stanc3/tree/master/test/integration/downstream, which is a good place to potentially put more of those models we want to make sure compile. I don't believe we're currently compiling that folder all the way down to a binary

@WardBrian
Copy link
Member

Seems like example-models/bugs_examples/vol1/seeds/seeds_stanified.stan performs some float multiply adds, which the optimizer detected and then did the wrong thing. The tests did their job here!

@rok-cesnovar rok-cesnovar requested a review from WardBrian May 3, 2022 17:21
@WardBrian WardBrian merged commit 19e1b27 into master May 3, 2022
@WardBrian WardBrian deleted the fix-compiler-compare-tests branch May 3, 2022 18:21
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.

2 participants