Make input Options case sensitive and error on unused Options #2210
Conversation
Constructing `Options` using an initializer_list like:
Options option {{"section1", {"value1", 42}}};
would not initialise the section names. This mostly down to the value
being constructed from the very bottom of the tree. When we construct
the bottom-most value, we don't know its parent's name, so we can't
give it its full_name "section1:value1".
Instead, we need to do this recursively, as we construct each parent.
Need all elements of Options::ValueType to have operator== in order to check equality of Options itself
Avoids segfault if run_rhs is called in Solver::solve
This is a breaking change! Set input:error_on_unused_options = false for old behaviour
This is a breaking change: input options are now case sensitive
ZedThree
changed the title
| @@ -661,6 +666,13 @@ public: | |||
| return *this; | |||
| } | |||
|
|
|||
| friend bool operator==(const Options& lhs, const Options& rhs) { | |||
| if (lhs.is_value and rhs.is_value) { | |||
There was a problem hiding this comment.
Is it worth having a shortcut here to check if they are the same object (&lhs == &rhs)?
| }; | ||
| append_impl(children, section_name, append_impl); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Is it worth instead making full_name a function, which iterates through its parents? Of course that raises the problem of setting pointers to parents... Another possibility is to not store the full name or parent, but that then puts it on wherever the full name is needed to work it out.
| // `(*this)[value.first].full_name` in the copy constructor, so we | ||
| // need to explicitly set it again | ||
| (*this)[value.first].full_name = value.first; | ||
| append_section_name((*this)[value.first].children, value.first); | ||
| } |
There was a problem hiding this comment.
Sorry! I missed this before. Maybe worth checking where full_name is actually needed, and if it could be worked out when needed e.g. as the tree of options is traversed.
There was a problem hiding this comment.
I needed it for printing out subsections correctly, i.e.
[section:subsection]
value = thing
but, yes, maybe this could be done when traversing the tree
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
As predicted, lots of test failures. It does look like a lot, if not all, are due to the case sensitivity: |
|
I've started looking through the failures. If we're going case-sensitive, we're going to need to decide on whether we want to keep existing spellings/cases or change them to match common usage. For example, the If we provide a tool for correcting case in input files, this should have minimal impact on users so I'm inclined to make most/all options in the library lowercase. |
Implemented using the "optimal string alignment distance" from Wikipedia: https://en.wikipedia.org/wiki/Damerau%E2%80%93Levenshtein_distance#Optimal_string_alignment_distance
|
Still WIP, and expecting tests to fail, as I've not yet fixed the input names, but errors should include suggestions TODO:
|
src/solver/solver.cxx
Outdated
|
|
||
| if (error_on_unused_options) { | ||
| Options unused = globaloptions.getUnused(); | ||
| if (not unused.getChildren().empty()) { |
There was a problem hiding this comment.
Maybe this whole chunk should be broken out into a function somewhere else? I'm still not keen on this living in Solver::solve, but moving it out requires something like BOUTMAIN calling PhysicsModel::init/run_rhs to get all the options.
Also the input:help functionality needs to essentially be called in the same place, and then we may have to be careful that nothing has tried to write to any files, or we'll end up clobbering stuff. I was wondering if we could somehow run in a "fake" location somewhere to avoid this. We'd still need access to the input file and grid file, etc.
There was a problem hiding this comment.
It might be nice to have the init and first RHS call be done in a separate call, rather than inside Solver. I remember a long time ago (2013?) we had to change how Solver initialised and ran, because of the weird interface PETSc had then. Perhaps its worth revisiting?
| @@ -109,6 +137,53 @@ const Options &Options::operator[](const std::string &name) const { | |||
| return it->second; | |||
| } | |||
|
|
|||
| std::multiset<Options::FuzzyMatch> | |||
| Options::fuzzyFind(const std::string& name, std::string::size_type distance) const { | |||
There was a problem hiding this comment.
There are various choices we could make about how this actually functions. I've chosen a default max distance of 4, so an option with more than 4 edits from the search string aren't counted, for example: some_string won't match another_string (by default).
It also matches an "unqualified name" (e.g. first) against a "fully qualified" name (e.g. mesh:ddx:first) with a distance of 1. But it won't try to strip the parent sections and match that. So mesh:ddx:first won't match some:other:first.
Another check is matching different cases: THIS matches this matches ThIs, all distance 1.
These choices were a little arbitrary, so definitely happy to hear suggestions on other choices
|
Might need to turn off clang-tidy-review on all the googletest stuff... |
* next: Note removing support for HDF5 in changelog Better notation for indexing zShift Update note on zShift in grid-files section of manual Print error for HDF5 macros when running bout-v5-macro-upgrader.py Support macros being removed in bout-v5-macro-upgrader.py Remove HDF5 support
|
Failing on unused options has a "fun" case where we have something like this: if (!estatic && !ZeroElMass) {
apar_solver = Laplacian::create(globalOptions->getSection("aparsolver"));
}Here, everything in the
More generically, do we expect the user to comment out or otherwise remove inputs when some other option means they go unused? Or to restructure their code to always read the options at least? Or perhaps to just confirm they've spelt everything correctly and then set |
|
I think an error message suggesting that the user checks, and then sets but with a better name. Perhaps an attribute in the |
This is a default argument to Options::getUnused to allow us to easily change what counts as having been used later
|
Ah, I had a feeling there was one, but I couldn't remember it off the top
of my head and searching for an example seemed tricky! Might need to do so
to find any others
We could transform the command line flag into something like
"restart:restart = true" when converting it to an Option. And we have the
tooling to change it in input files too. There would be a discrepancy
between setting it on the command line and in an input file, which is a bit
unpalatable, but maybe not too much?
…On Thu, 8 Apr 2021, 18:51 Ben Dudson, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/sys/options.cxx
<#2210 (comment)>:
> }
}
- }
- for (const auto &it : children) {
- if (it.second.is_section) {
- it.second.printUnused();
+
+ // What is it doing here?!
I hoped to get rid of this too, but at the moment we have a top level
option restart and a section of the same name
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2210 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALLAXVZOHVLR23EALGZLQTTHXUKBANCNFSM4WENBINA>
.
|
* next: (159 commits) CMake: Add 'docs' synonym for sphinx-html target CMake: Add PYTHONPATH env var to docs command CMake: Use correct latex builder invocation CMake: Fix missing 'echo' in docs command Docs: Fix use of deprecated/removed Sphinx method Remove extraneous `this->` when setting member variables Fix a couple of typos in tests Only check local y points if we have multiple processors in Y Docs: Fix sphinx conf.py for breathe 4.30 CMake: Print link to generated docs after completion CMake: Fix typo in docs comment [skip ci] Apply black changes Print possible boundary regions during startup Add Mesh method to get the set of all possible boundaries Move constraint creation to utility method CMake: Default to not building the docs CMake: Be explicit that docs aren't built in `all` CMake: Fix typo Make method parameter names consistent (clang-tidy fix) Check version of SUNDIALS is new enough for constraints ...
examples/boutcore/data/BOUT.inp
Outdated
| mxg = 1 | ||
| myg = 1 | ||
| MXG = 1 | ||
| MYG = 1 |
There was a problem hiding this comment.
Can these top-level options be removed? There are MXG, MYG settings in the mesh section too
There was a problem hiding this comment.
If the top-level options are removed, the mesh section ones are used, and the simulation fails.
I'll push a fix.
There was a problem hiding this comment.
Lovely, thanks @dschwoerer 👍
|
I'd just like to implement the fix for the boundary condition checking before we merge this. Hopefully won't take too long! Also, it's not necessary for this PR, but merging #2277 would also allow a small simplification here. |
* next: (58 commits) Report enabled sanitizers in final config summary Fix clang-tidy suggestions: move-assignment should be noexcept Don't std::move trivially-movable objects Avoid calls to virtual functions in constructors Handle self-assignment in copy-assignment Make move-assignment operator noexcept Use explicit nullptr check in conditional Remove else-after-return Fix use-after-move in Field3D move-assignment Add move-assignment for Field2D Fix for clobbering boundary operators in field assignment Fix for `setBoundary` being called before `fieldmesh` has been set Use local mesh in `FieldData` methods Don't use reserved identifiers as header guards in fields Unconditionally include string header for `Field` Update docstrings for `Field` and `FieldData` classes Make most `FieldData` members private with getters, not protected Swap `FieldData` members in `FieldData` Make `setDirectionY` virtual Make `Field::directions` private ...
We now (soft) error if there are any unused input parameters, but because the set of boundary regions can be different on different processors, we might not "use" (that is, read the value of) the corresponding input parameters on the other processors! This function is a bit of a workaround for the above: essentially marks all possible boundary condition inputs as having been used on each processor. This way, we should still catch invalid inputs and misspellings (for example, `bdry_oll`) while still allowing valid boundaries on other processors.
These aren't needed as boundaries are set manually on `f`
Now covered in library
|
Should've just merged this earlier, instead of trying to fix it all in this PR! It looks like the failing tests are because |
Turns out creating `Region`s on production-sized meshes is expensive. We don't need them in this function, so don't bother computing them!
|
It turns out the biggest cost of |
* next: (63 commits) Use better variable names Add downloading dependencies to CMake install doc CMake: Add option to download SUNDIALS at configure time Delete some commented out code Remove some unused private members Fix clang-tidy warning about uninitialised members Use override on overridden virtual functions Make some LaplacePCR methods const Reduce scope of some local variables and immediately initialise them Fix some clang-tidy warnings Fix use of reserved identifier for header guard Keep const in cxx Remove const int input Add PCR case to the test-laplace integrated test clang format Allow NYPE != 1 Inline the setup function Use GlobalNxNoBoundaries Tidy comment Run clang tidy ...
As discussed in #2209, this makes
Optionscase sensitive, which is a breaking change.It also introduces another breaking change: by default, any unused options after an initial call to
rhsis an error. This can be toggled withinput:error_on_unused_options(I used this instead ofinput:strictas it's more explicit:strictsounds like turning it off might allow case-insensitive options).I had to fix some other bits for this to work: some
Solverunit tests were a little fragile, and constructingOptionsfrom a braced init list turned out to be broken in a fun way. WritingOptions::getUnusedalso turned out to be way more difficult than I was expecting.Optionsis actually a little more difficult to use than I thought.I'm not overly happy with this check being done in
Solver::solve-- it's the obvious place, but it doesn't really feel likeSolver's responsibility. I want to also addinput:helpwhich prints out all the seen options, and again,Solver::solveis the obvious place, but it feels like it should live somewhere "higher up".Also to be done, add Levenshtein distance checking to point out likely typos
Also, I've added
toString(Options)which ignores the doc strings and source. It's very close to just completely replacingOptionsINI, so I'd also like to work out how to merge all that.I figure all these changes are very likely to break some tests, so I'm looking forward to seeing what needs fixing!