-
Notifications
You must be signed in to change notification settings - Fork 3
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
Put benchmark boilerplate into subpackage #136
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 87.23% 90.24% +3.00%
==========================================
Files 31 36 +5
Lines 1363 1455 +92
==========================================
+ Hits 1189 1313 +124
+ Misses 174 142 -32 ☔ View full report in Codecov by Sentry. |
Isn't that what the |
No, the benchmark folder is for benchmarking only, and has deps like BenchmarkTools. This code is used in tests, benchmarks and other places too. And it needs to be in a package to be importable in e.g. a Pluto notebook (to which I can give the exact GitHub URL of |
The name may be ill-chosen, perhaps |
Not trying to be snarky here, but isn't
Is that a big issue? I think removing some complexity at the cost of one dependency is acceptable.
Wouldn't |
At the very least, we could move this into |
See my latest comment, here I meant "benchmarks" in the sense of "benchmark instance definitions", not "full specification of a benchmark suite with sizes and evals and so on". Hence the possibility of an alternative name like |
This is pretty much a constant-complexity PR. It's just replacing the
It's not just about giving a URL, it's about giving the URL of a package (as opposed to including a file) |
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" | ||
ReferenceTests = "324d217c-45ce-50fc-942e-d289b448e8cf" | ||
SimpleDiffEq = "05bca326-078c-5bf0-a5bf-ce7c7982d7fd" |
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.
Why did this have to be added?
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.
because it's used in the benchmarks
@adrhill the benchmarks work on this PR branch and fail on main but that's expected, this is good to merge |
Some code defining problem instances is used by tests and benchmarks, as well as external notebooks. Putting it in a package dispenses us from using
include
every time, and brings cleaner dependency management, at the cost of aPkg.develop
.benchmarks/SparseConnectivityTracerBenchmarks
to put:@benchmarkable
just once in the test suite