ParameterHandler::parse_input_from_json(): do not use mangled parameter names anymore
#16544
Conversation
There was a problem hiding this comment.
After #13815, this PR is the only logical step to have a consistent input and output for the JSON format. Thank you.
The function no longer works for mangled JSON files, correct? I can imagine that this would be a big incompatibility for some users (including aspect? @gassmoeller @tjhei, referring to #13815 (comment)).
Is there a way to check whether a tree is mangled? Is there a way to convert a json file from mangled to demangled (that you could mention in the changelog)? This could be helpful for the transition.
source/base/parameter_handler.cc
Outdated
| recursively_mangle_or_demangle(p.second, | ||
| temp, | ||
| is_parameter_node(p.second) || | ||
| is_alias_node(p.second)); | ||
| tree_out.put_child(demangle(p.first), temp); |
There was a problem hiding this comment.
So demangle works bidirectional from mangled to demangled and vice versa? It isn't obvious to me how the mangle part in recursively_mangle_or_demangle works. Could you elaborate (maybe with in-source commentary)?
There was a problem hiding this comment.
There was something missing in the old state of the code. Now, it should work bidirectional since we perform either mangling/demangling depending on the flag do_mangle. I rephrased the documentation accordingly.
|
ASPECT doesn't use JSON. I think the compatibility issue is not huge. I'm ok with the incompatibility assuming it is properly documented in the changelog. I had been under the impression that keys in JSON cannot have spaces in it, but looking at the changed output files, this seems to work just fine because both keys and values (if necessary) can be put in double quotes. So this is fine with me. |
|
@mschreter Ping? |
mschreter
force-pushed
the
parse_input_from_json_mangle
branch
from
fe8535d
to
717976f
Compare
|
Thank you for your review @marcfehling.
Correct, after this change json files are always assumed to be demangled. If this is a big incompatibility, we probably need to extend this PR.
There is certainly a way to determine the state of the json file, but I am not sure if this is really what we want and I don't feel qualified enough to make this decision :-). However, if I interpret @bangerth's comment correctly, he would be fine with the incompatibility break in terms of requiring demangled json files in the future. Or do you @marcfehling/@bangerth have any specific use cases in mind where this PR could have a huge impact? |
|
@bangerth we use the JSON parameter output in ASPECT to generate the markdown documentation and this depends on mangling: https://github.com/geodynamics/aspect/blob/main/contrib/utilities/jsontomarkdown.py This PR would break this. It wouldn't be hard to fix, but it is a bit annoying because we need to know what deal.II version was used. |
|
That said, I am ok with changing it. It makes sense. Is there a way to see which deal.II version wrote the file? If not, can we add this inside the output? |
|
JSON does not allow for comments (one of the more startling decisions in the software world), so we cannot write the version number into a comment as we do in other file formats. We could put it into a "magic" variable, but that has the disadvantage that it is not backward compatible because previous version's readers do not know about the magicness of the variable. |
True. It would also have the disadvantage that parsing the comment would be more difficult.
Sadly we did not put all the output inside a "parameters" (or similar) block. Then we could include a version string. While breaking everybody, this would allow adding additional information... |
|
/rebuild |
|
How shall we proceed @bangerth @tjhei? Another point which I would like to mention: The current implementation prints parameters in the *.json format in a demangled state. So we cannot read them anymore as long as we require mangled *.json files for parsing. I am afraid we have to fix this incompatibility one way or the other. |
|
ping :) |
|
I am okay updating to using demangled read/write. How do you feel about moving all entries into a single "parameters" root? That would allow us to add other information in the future (version info, etc.). You can even detect the two kinds of formats when loading (if there is a "parameters" root, you know you have the new version). |
@tjhei I am afraid that moving everything into a new section/node will break all json parameter files used by the users. This patch only fixes an inconsistency between reading/writing json files. At the moment we can write json files (by deal.II) in such a way that we cannot read it anymore. |
|
@mschreter Could you rebase the PR? I think after that we can merge 👍 |
Co-authored-by: Magdalena Schreter-Fleischhacker <schreter.magdalena@gmail.com>
mschreter
force-pushed
the
parse_input_from_json_mangle
branch
from
717976f
to
b8ef99c
Compare
|
If there are no objections, I will merge this PR tomorrow. |
As described in #16453 parameter names in
*.jsonneeded to be mangled. In this PR, we suggest to not use mangled parameter names in a*.json-file anymore. We mangle them internally to use the existing XML reading functionality.I also renamed the corresponding tests to keep the naming consistent, which was not the case before.
closes #16453