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

Multigrid config #1480

Merged
merged 9 commits into from
May 30, 2024
Merged

Multigrid config #1480

merged 9 commits into from
May 30, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Nov 30, 2023

This pr adds the config interface for multigrid and coarsening method.

@yhmtsai yhmtsai self-assigned this Nov 30, 2023
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. type:solver This is related to the solvers type:multigrid This is related to multigrid labels Nov 30, 2023
@yhmtsai yhmtsai force-pushed the prec_fact_config branch 2 times, most recently from 0b19c10 to 91840c1 Compare November 30, 2023 23:21
@yhmtsai yhmtsai force-pushed the multigrid_config branch 2 times, most recently from 72462d6 to 9df1b83 Compare December 1, 2023 09:06
Copy link

sonarcloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

69.0% 69.0% Coverage
1.3% 1.3% Duplication

warning 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.
Read more here

Copy link
Member

@MarcelKoch MarcelKoch left a 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.

const config::type_descriptor& td_for_child)
{
auto factory = FixedCoarsening<ValueType, IndexType>::build();
// TODO: ARRAY
Copy link
Member

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 Show resolved Hide resolved
core/solver/multigrid.cpp Outdated Show resolved Hide resolved
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*) {
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

Comment on lines 496 to 610
factory.with_mg_level(
gko::config::get_factory_vector<const gko::LinOpFactory>(
obj, context, td_for_child));
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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)

core/multigrid/fixed_coarsening.cpp Outdated Show resolved Hide resolved
@yhmtsai yhmtsai force-pushed the multigrid_config branch 2 times, most recently from 528d27e to 060a800 Compare May 28, 2024 13:21
Base automatically changed from prec_fact_config to develop May 28, 2024 19:13
@yhmtsai
Copy link
Member Author

yhmtsai commented May 29, 2024

@thoasm the documentation relies on two parts

  1. general rules in parse of config.hpp: it gives the idea about different type of setting.
  2. detail option and setting in factory parameters: aim for same support but little difference between code and config

@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.

@yhmtsai yhmtsai requested a review from MarcelKoch May 29, 2024 06:57
@yhmtsai yhmtsai force-pushed the multigrid_config branch 3 times, most recently from 8c53d45 to 1995ba1 Compare May 29, 2024 10:36
@yhmtsai yhmtsai force-pushed the multigrid_config branch 2 times, most recently from 3703a50 to d92ac0c Compare May 29, 2024 12:55
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 29, 2024
@yhmtsai yhmtsai force-pushed the multigrid_config branch 2 times, most recently from 50abb16 to f91b704 Compare May 29, 2024 17:44
@yhmtsai yhmtsai merged commit 6e3f9e3 into develop May 30, 2024
12 of 15 checks passed
@yhmtsai yhmtsai deleted the multigrid_config branch May 30, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. reg:build This is related to the build system. reg:testing This is related to testing. type:multigrid This is related to multigrid type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants