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.

Sign up for GitHub

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

Add triangular solvers to benchmark #664

Merged
merged 2 commits into from
Nov 20, 2020
Merged

Add triangular solvers to benchmark #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
867fdfe
add triangular solvers to benchmark
upsj Nov 19, 2020
f25a8d7
review update
upsj Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions BENCHMARKING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ The supported environment variables are described in the following list:
library formats (cuSPARSE with `cusp_` prefix or hipSPARSE with `hipsp_`
prefix) can be used as well. Multiple options can be passed. The default is
`csr,coo,ell,hybrid,sellp`.
* `SOLVERS={bicgstab,bicg,cg,cgs,fcg,gmres}` - the solvers which should be
benchmarked. Multiple options can be passed. The default is `cg`.
* `SOLVERS={bicgstab,bicg,cg,cgs,fcg,gmres,lower_trs,upper_trs}` - the solvers
which should be benchmarked. Multiple options can be passed. The default
is `cg`. Note that `lower/upper_trs` by default don't use a preconditioner,
as they are by default exact direct solvers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether this is necessary, but should we mention that lowe/upper_trs expect a triangular matrix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Too late sorry, the PR is already merged 🤷‍♂️

Copy link
Contributor

@Slaedr Slaedr Nov 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Essentially, I guess we should define what are 'lower_trs' and 'upper_trs', in case people are not familiar. From an applications point of view, or indeed from most common points of view, these two are really not in the same category as bicgstab, gmres, etc. We should address this in some other PR.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have to explain everything about the TRS here necessarily, because this here is only the benchmarks description. If you want information from a user POV, the class description etc should give you all you need. On the other hand, someone who reads this should know what to benchmark, and only wants to know how.

* `SOLVERS_PRECISION=<precision>` - the minimal residual reduction before which
the solver should stop. The default is `1e-6`.
* `SOLVERS_MAX_ITERATION=<number>` - the maximum number of iterations with which
Expand Down
22 changes: 18 additions & 4 deletions benchmark/solver/solver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ DEFINE_bool(
rel_residual, false,
"Use relative residual instead of residual reduction stopping criterion");

DEFINE_string(
solvers, "cg",
"A comma-separated list of solvers to run. "
"Supported values are: bicgstab, bicg, cg, cgs, fcg, gmres, overhead");
DEFINE_string(solvers, "cg",
"A comma-separated list of solvers to run. "
"Supported values are: bicgstab, bicg, cg, cgs, fcg, gmres, "
"lower_trs, upper_trs, overhead");

DEFINE_uint32(
nrhs, 1,
Expand Down Expand Up @@ -167,6 +167,20 @@ const std::map<std::string, std::function<std::unique_ptr<gko::LinOpFactory>(
.with_preconditioner(give(precond))
.on(exec);
}},
{"lower_trs",
[](std::shared_ptr<const gko::Executor> exec,
std::shared_ptr<const gko::LinOpFactory>) {
return gko::solver::LowerTrs<>::build()
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, we dont use a template type here ? Maybe it is better to add ValueType to all template parameters ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a valid point! However, I believe we should address this in another PR

.with_num_rhs(FLAGS_nrhs)
.on(exec);
}},
{"upper_trs",
[](std::shared_ptr<const gko::Executor> exec,
std::shared_ptr<const gko::LinOpFactory>) {
return gko::solver::UpperTrs<>::build()
.with_num_rhs(FLAGS_nrhs)
.on(exec);
}},
{"overhead", create_solver<gko::Overhead<>>}};


Expand Down