[Param Update 2 of 2] Eager Parameter Parsing#505
Open
mabruzzo wants to merge 41 commits into
Open
Conversation
``AttrRecorderInterface`` is an abstract class that defines a generic interface that different file backends may implement in order to save file-header-attributes. - subclasses are expected to implement the pure virtual functions - the existing ``H5AttrRecorder`` class has been tweaked so that it is now a subclass of ``AttrRecorderInterface`` ``Write_Header_`` is a local function that operates upon a reference to ``AttrRecorderInterface`` (in practice, callers need to pass an instance of a concrete subclass). The implementation was extracted from ``Grid3D::Write_Header_HDF5``
It is now known as ``always_false`` (I have modified the docstring to further clarify its purpose)
…y an error when the test was written
I also changed the namespace that they are a part of
Previously we passed all cli arguments to ParameterMap. This caused some issues now that our parsing is getting a little more strict
This means Cholla will more eagerly reject invalid parameter files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should not be reviewed until after #503 is merged (if it's merged at all)
This PR updates the internals of
ParameterMapto eagerly parse in parameter values while reading in a Parameter File. This sort of eager parsing is only possible now that #503 has been merged.Additional Context
Prior to #503 the intended types of the values in the following snippet were ambiguous:
To be a little more specific,
truecould be a boolean or string,1could be an integer or string, and1.0could be a float or a string. Because of this ambiguity,ParameterMaphad to be very lazy about converting values from the original strings. In other words,ParameterMapwould have stored std::string instances holding"true","1", and"1.0". It only tried to parse the value when when the parameter was actually accessed.The change in PR #503 removed this ambiguity since strings must now be quoted (in other words, none of the values in the snippet can be string values under the new rules). For minimized the size of the proposed changes, PR #503 remained quite lazy about when the parameter values are parsed.
This PR proposes changes where the values of parameter files are immediately parsed. They are stored in a new type called
param_details::Value, which is a safe union that directly holds either abool,int64_t,double, orstd::string.Why make this change
There are several lines of motivation for making this change:
Cholla can now immediately reject a lot more parameter files that are written with invalid syntax (under the old-system we could only reject ill-formed values if we actually tried to read it through its associated key). I think this is actually a really big deal.
This change makes it possible to go back and revisit PR WIP: Dumping hdf5 parameters #409. As a reminder, that PR records parameters inside an hdf5 file. In my mind it makes a lot more sense to record values after they have been converted to bool/double/int/properly escaped strings than to store the original raw strings from the parameter file (the raw-string plan would require you to use a toml library to actually use the dumped parameters)
This is an essential step to using an external toml-parsing library (e.g. it should be straight-forward to replace our
param_details::Valueclass withtoml::valuefrom toml++)Finally, this is a pretty crucial step to introducing alternative command line argument syntax to more easily override string parameters
outdir:str=blahwhere:strwould make it clear thatblahis a stringJust like #503, this will fail tests until we convert all the parameter files for the system tests using the script provided with #503.