-
Notifications
You must be signed in to change notification settings - Fork 91
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
Multigrid config #1480
Multigrid config #1480
Conversation
0b19c10
to
91840c1
Compare
72462d6
to
9df1b83
Compare
91840c1
to
03cc9db
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs 69.0% Coverage The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
9df1b83
to
2509e65
Compare
03cc9db
to
906e8b5
Compare
b598f26
to
367d615
Compare
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.
This looks mostly good, but I have two requests:
- don't provide configuration for
FixedCoarsening
, it doesn't have any meaningful parameters for the configuration - don't provide the selector parameter for the multigrid. They are too advanced to be used frequently, so we can take a bit more time to find a representation in our config file for them.
core/multigrid/fixed_coarsening.cpp
Outdated
const config::type_descriptor& td_for_child) | ||
{ | ||
auto factory = FixedCoarsening<ValueType, IndexType>::build(); | ||
// TODO: ARRAY |
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.
Without the array, the fixed coarsening is pointless. I would also suggest, that we shouldn't support this type of parameter in our config anyway, since they very explicitly depend on the concrete sizes of the matrices. Those are not (and probably should not) be available in the config, so we can't have parameters that depend on them.
core/solver/multigrid.cpp
Outdated
static std::map<std::string, | ||
std::function<size_type(const size_type, const LinOp*)>> | ||
selector_map{ | ||
{{"first_for_top", [](const size_type level, const LinOp*) { |
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.
I'm not sure this key is understandable. In general, I would suggest to not support the level/solver selector at the moment. It is again something that is very complex to use, but has very few potential use cases. I would not expect our users to actually use this.
So I would give this more thought and add it in the next release.
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.
I can provide another default option without settings, which I think should be more practical for most cases.
use mg_level.at(level)
when level < vector_len
and reuse the last one if level >= vector_len
.
It may be more suitable in the default selector not in config
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.
That doesn't help. The selector should not be part of this release. It's still too complex with very little use cases.
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.
I have created #1611 and remove the selector now
core/solver/multigrid.cpp
Outdated
factory.with_mg_level( | ||
gko::config::get_factory_vector<const gko::LinOpFactory>( | ||
obj, context, td_for_child)); |
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.
without the selector, this should not use vectors. Same for the other instances here.
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.
By default, if user gives the enough mg_level, which less than the generated levels (or limited max_mg_level).
Multigrid will use each for each level
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.
I would still advocate to only allow a single value here. How many mg_levels users need to provide should again depend on the matrix, so it would be unknowable for the configuration.
Also going to multiple levels later would be backwards compatible, so there would be no issue.
(same goes for the other vector parameters here)
57d3df4
to
0b77935
Compare
528d27e
to
060a800
Compare
@thoasm the documentation relies on two parts
@MarcelKoch I have removed the selector, but I still keep the vector input. User can still use it by one argument. Default selector still works normally when they give many. They can also input the selector after file config otherwise. |
8c53d45
to
1995ba1
Compare
no array support so fixed_corsening is useless currently
Co-authored-by: Marcel Koch <[email protected]> Co-authored-by: Thomas Grützmacher <[email protected]>
3703a50
to
d92ac0c
Compare
50abb16
to
f91b704
Compare
This pr adds the config interface for multigrid and coarsening method.