-
Notifications
You must be signed in to change notification settings - Fork 86
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
add file config example with the configs #1662
base: develop
Are you sure you want to change the base?
Conversation
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.
LGTM. I would prefer the json files merged.
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.
IMO, all json files should be merged, and the filenames should be keywords instead. I think that is more realistic for acutal usecases.
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 thought user usually swap the file to choose the solver, which gives less confusing.
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 think a single file is less confusing, since it's only one file, where the keys are the previous filenames, and you can add comments for each configuration. (Ofc not in JSON, but that is arguably not a good format for this use case anyway.)
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.
After reading through this PR, I agree with @yhmtsai.
Having one file per configuration is much simpler because it makes it easier to swap them around.
Additionally, it is easier to understand when reading the code, which is important for this example.
std::ifstream f(configfile); | ||
std::cout << f.rdbuf() << std::endl; |
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.
You could output the json directly:
std::cout << std::setw(4) << nlohman::json::parse(configfile) << std::endl;
see: https://json.nlohmann.me/api/operator_ltlt/
Of course, it would make more sense if the json object was stored already before.
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.
Thanks for this information.
I will put it when we have the example with loading json object before
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.
Right now, I only comment until the discussion is resolved.
I would like all configuration files to be tested and part of the CI.
Other than that, I only have nits.
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.
After reading through this PR, I agree with @yhmtsai.
Having one file per configuration is much simpler because it makes it easier to swap them around.
Additionally, it is easier to understand when reading the code, which is important for this example.
// mixed-pgm-multigrid-cg.json: mixed-multigrid-preconditioned-solver | ||
// (assuming there are always more than one | ||
// level) | ||
auto config = gko::ext::config::parse_json_file(configfile); |
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.
My main concern is that this only tests a single file as part of our automatic tests. It doesn't check the other files, so we couldn't detect if we broke the compatibility.
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 am not sure whether using examples as kind of integration test is a good idea.
I have add the test to check they can be runnable
@@ -20,6 +20,7 @@ set(EXAMPLES_EXEC_LIST | |||
set(EXAMPLES_LIST | |||
${EXAMPLES_EXEC_LIST} | |||
custom-stopping-criterion | |||
file-config-solver |
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.
Does this mean they are not run as part of our CI?
Is there a reason why we don't?
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 added test which only checks it can run properly.
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 do not reuse the existing one because it needs to accept additional parameter
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
e25ce2f
to
e096a21
Compare
Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@kit.edu>
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 still think that
- JSON is not the right format
- The files should be combined, and annotated with comments
But I won't hold up the PR.
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.
you also need to add the file to doc/examples/examples.hpp.in
This PR adds the example to show the solver configuration based on the file.
This PR aims to set up config close to the existing example without any registry usage.
Note. the stopping criterion is different among these configs.
There will be another one example to show the registry usage.