-
Notifications
You must be signed in to change notification settings - Fork 4
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
Issue 357: Move from fixed n with missing to flexible n #392
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will be interested if this has any noticeable effect on benchmarks.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
=======================================
Coverage 93.33% 93.33%
=======================================
Files 53 53
Lines 540 540
=======================================
Hits 504 504
Misses 36 36 ☔ View full report in Codecov by Sentry. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
08ca35f
to
f6efb72
Compare
I've added integration benchmarks (including from #357 which now run without check warnings being thrown (as far as I can see). I haven't added new unit tests as I felt like the functionality change was covered by the benchmarks but can cover in a follow up PR if we need to? |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/Rt-without-renewal/Rt-without-renewalJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the extra tests.
This PR expands on the ideas in #389 and from f2f conversations to try to partially resolve some issues seen in #357. Before merging it needs a benchmark and test update to check that it does this.
I think from this implementation their are few user side downsides apart from potentially some confusion about the changing length of complex latent models.