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

ParameterHandler::print_parameters(): Demangle JSON #13815

Merged
merged 1 commit into from May 25, 2022

Conversation

mschreter
Copy link
Contributor

@mschreter mschreter commented May 25, 2022

In this PR, we propose to demangle the parameters in case of ParameterHandler::OutputStyle::JSON or ParameterHandler::OutputStyle::ShortJSON prior to printing them within ParameterHandler::print_parameters(). Otherwise, the output is not really readable, e.g., for the key-value pair of

"end time": "0.5"

printing the output ends up in

"end_20time": "0.05"

@peterrum @nmuch

Co-authored-by: Peter Munch <peterrmuench@gmail.com>
@mschreter mschreter changed the title ParameterHandler::print_parameters(): Demangle JSON during output ParameterHandler::print_parameters(): Demangle JSON May 25, 2022
@peterrum
Copy link
Member

/rebuild

@peterrum peterrum added this to the Release 9.4 milestone May 25, 2022
@bangerth bangerth merged commit 3479b9a into dealii:master May 25, 2022
@gassmoeller
Copy link
Member

I think this PR is causing some unintended consequences in ASPECT, but I am not sure if it is deal.II's fault or how we use it. Our parameters contain a bunch of keys named "deprecation_status", which are apparently created by the function ParameterHandler::declare_alias. I guess the underscore looks to the function as if it needs to be demangled, but then it fails. @mschreter could you check if this change works with a parameter alias for you?
I was successful reproducing this problem by adding the following line after line 40 of tests/parameter_handler/parameter_handler_write_json.cc:

      prm.declare_alias("double 2", "double 3");

I guess in the end the question is: Are underscores valid characters for parameter names in deal.II? If so, can we change this modification to not accidentally try to demangle these underscores?

@tjhei
Copy link
Member

tjhei commented Jun 3, 2022

Makes sense. I can take a look this weekend if nobody else beats me to it l.

@mschreter
Copy link
Contributor Author

@gassmoeller Thanks for your notice. Unfortunately I did not test the behavior in case of an alias parameter. I was able to reproduce your issue and hopefully also already found the problematic line, see this patch. Now, the diff of the test output oftests/parameter_handler/parameter_handler_write_json.cc including your suggested changes looks as follows:

65,69d64
<             },
<             "double 3":
<             {
<                 "alias": "double 2",
<                 "deprecation_status": "false"
89,94c84
<             "double 2": "4.321",
<             "double 3":
<             {
<                 "alias": "double 2",
<                 "deprecation_status": "false"
<             }
---
>             "double 2": "4.321"

If you are happy with that, I'll open a follow-up PR including the fix and also an extension of the test case with alias parameters.

FYI @tjhei

@tjhei
Copy link
Member

tjhei commented Jun 4, 2022

That looks good! Thank you.

@gassmoeller
Copy link
Member

Looks good to me, feel free to open a PR. Thanks for the quick fix! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants