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

Fix xmlreader allowed bool values #286

Merged

Conversation

cniethammer
Copy link
Contributor

@cniethammer cniethammer commented Feb 8, 2024

Description

This is part of the work for #255 addressing the checks for the shifted parameter in the input files.
The way this is fixed here is by making the xmlreader generally more strict in parsing boolean input values.

  • Fix xmlreader to only accept true/false, yes/no or on/off as boolean values now

  • Fix examples to use false and true as boolean values in xml input files where they failed to

  • Fix unit tests to use false and true as boolean values in xml input files where they failed to

  • Added the new DropletCoalescence example to list of standard inputs to be tested

  • Tested running all examples in the "examples/example-list.txt" for 10 time steps

  • Tested running internal unit tests

…values

Do not allow integer values 0 or 1 as boolean values from input to prevent
possible misunderstandings, e.g., in case of the shifted parameter a value
of zero could be intended.
Updated error message for users accordingly to help fixing input files.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer cniethammer requested a review from HomesGH February 8, 2024 12:20
@cniethammer cniethammer force-pushed the fix_xmlreader_allowed_bool_values branch from 99c5284 to c3d8911 Compare February 8, 2024 12:56
Copy link
Contributor

@HomesGH HomesGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change since it improves the readibility of the config files. However, it will probably break a lot of configuration files of the users in the beginning.
In addition, also the "documentation" in the header files should be updated, e.g. here. Is this "documentation" in the header also parsed by Doxygen?

@cniethammer
Copy link
Contributor Author

Thanks for the thorough review.

Yes, this might cause users to update their configuration files. However, no existing functionality is removed by these changes. Also, the user will get an error message and appropriate information on what to change in his configuration file.

Yes, Doxygen parses all source files as well as a few files in the doc directory for the generation of the documentation.
Concerning the documentation I updated the mentioned one as well as a few other places.

@HomesGH
Copy link
Contributor

HomesGH commented Feb 12, 2024

Alright, thank you. We could also search for something like >0< or >1< to get more locations where it should be changed, e.g. in the planar LRC.

…bool values in xml

Signed-off-by: Christoph Niethammer <[email protected]>
…ter to use true/false for bool values in xml

Signed-off-by: Christoph Niethammer <[email protected]>
…se true/false for bool values in xml

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer
Copy link
Contributor Author

I checked now the codebase with the following grep command for possible issues

grep -E -v '(Ixx|Iyy|Izz|start|stop|component|smooth|vx|vy|vz|<x>|<y>|<z>|framesperfile|<b>|<g>|<r>|currenttime|steps|refID|rebalanceLimit|ParticlesBufferSizeMPI|temperature|lcx|lcy|lcz|yPos|ymin|binindex|direction|yl|adios2step|left|init)'

This has one items left that is not covered with this PR:

I thereby consider this PR as complete

@HomesGH
Copy link
Contributor

HomesGH commented Feb 13, 2024

I checked now the codebase with the following grep command for possible issues

grep -E -v '(Ixx|Iyy|Izz|start|stop|component|smooth|vx|vy|vz|<x>|<y>|<z>|framesperfile|<b>|<g>|<r>|currenttime|steps|refID|rebalanceLimit|ParticlesBufferSizeMPI|temperature|lcx|lcy|lcz|yPos|ymin|binindex|direction|yl|adios2step|left|init)'

This has one items left that is not covered with this PR:

* The 'adaptiveContainer' - this is fixed separately in [Fix type of _adaptive parameter in FMM to bool #288](https://github.com/ls1mardyn/ls1-mardyn/pull/288)

I thereby consider this PR as complete

Very nice! 👍

@HomesGH HomesGH self-requested a review February 13, 2024 11:52
@cniethammer cniethammer merged commit 49da39a into ls1mardyn:master Feb 13, 2024
52 checks passed
@cniethammer cniethammer deleted the fix_xmlreader_allowed_bool_values branch February 13, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants