Skip to content

Check config key #1792

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Check config key #1792

wants to merge 8 commits into from

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Feb 18, 2025

This PR adds the check to verify the keys in config are allowed by predefined keys set.
typo in config is quite easy in the file because we do not have auto-complete for that
It avoids typo or misusage of parameters from user such that the configuring solver is not what users expect without notice.

Isome thoughts will be nice.
allowed_keys preparation: predefine it as early as possible or insert the key into the set close to the condition
I am predefining it as early as possible. but I think the second way might makes sense when the class has long list of keys like multigrid. It makes the manual type together and reduce the chance that missing deleting one of condition and allowed key. The first one is somehow easy to set with constructor not many lines of set.insert("key")

@yhmtsai yhmtsai added the 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. label Feb 18, 2025
@yhmtsai yhmtsai requested a review from a team February 18, 2025 16:01
@yhmtsai yhmtsai self-assigned this Feb 18, 2025
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. type:solver This is related to the solvers labels Feb 18, 2025
@yhmtsai yhmtsai added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. labels Feb 20, 2025
@yhmtsai yhmtsai requested a review from fritzgoebel February 20, 2025 15:40
@yhmtsai yhmtsai force-pushed the check_config_key branch 3 times, most recently from 6e141fe to db95be4 Compare March 4, 2025 15:41
@MarcelKoch MarcelKoch added this to the Ginkgo 1.10.0 milestone Mar 13, 2025
@MarcelKoch
Copy link
Member

Suggestion: use the pnode to keep track of which keys of a map were accessed. If not all keys were accessed, then there are some invalid keys.
The pnode will also keep track of what is the set of allowed keys, since the parser function will test for all valid keys.

@upsj
Copy link
Member

upsj commented Mar 13, 2025

Implementation example:
Replace

if (auto& obj = config.get("max_iters"))

by

std::set<std::string> allowed_keys;
auto get_config_value = [](const char* key) {
    allowed_keys.insert(key);
    return config.get(key);
};
// ...
if (auto& obj = get_config_value("max_iters"))

The main goal is to specify every key name string only once, so we avoid accidental typos.

@yhmtsai
Copy link
Member Author

yhmtsai commented Mar 18, 2025

It merge the allowed keys preparation with trying getting nodes now

@yhmtsai yhmtsai force-pushed the check_config_key branch from 2234682 to 526654a Compare May 26, 2025 11:05
@yhmtsai yhmtsai force-pushed the check_config_key branch from 526654a to 2538b02 Compare May 26, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module. reg:testing This is related to testing. type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants