-
Notifications
You must be signed in to change notification settings - Fork 39
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
Mixed input file and C++ API options handling #569
Comments
If the user passes an options object, the input file never gets parsed. Probably want to mess around with that logic. |
@dmcdougall @roystgnr For the Environment (and other) constructors that accept both input file name and EnvOptionsValues object, what do you feel the precedence should be? On the Dakota side, we're planning to have the C++ options object apply first, followed by the parsed input file options as this lets a user write a C++ program with their preferred defaults possibly overriding QUESO class defaults, but then lets power users override them at run-time through a file. So our thinking is (1) class default values, possibly overridden by (2) explicitly set options in C++ options object, possibly overridden by (3) user input file. |
Per @dmcdougall, also need to consider where in this workflow a client might programmaticly set options as well, e.g., to override defaults through C++ code. |
Although the goal is to give the user as much control over the ordering as possible, there's no way to do that when you have two options sources going into the same constructor, and "C++ object gets used first, but input file can override anything in it" is clearly the way to go. We can make sure there's accessors available if anyone wants to then override an input file option from C++ even later. |
Partially addresses libqueso#569 Following the lead from GPMSAOptions, refactor EnvOptionsValues to allow construction from defaults vs. passed options object, with optional override from parsed user input file. Finer grained construct, set_defaults(), parse(), further allow a client to intersperse setting options via C++ as well since member variables are public. This commit changes a couple historical behaviors: * If there was an inbound EnvOptionsValues object, the input file never got parsed. * An Environment could have a NULL m_optionsObj; now we construct one with default values. This could lead to subsequent code cleanup. Retains optional boost::program_options parser for now.
Partially addresses libqueso#569 by addressing feedback in PR libqueso#640, specifically: * Keep comments in header * Prefer reference to pointer * Cleanup m_optionsObj tests for NULL (adding require !NULL in a couple places) * Only initialize BoostInputOptionsParser when needed * Take care not to have trailing whitespace when converting set of integer to string Some tests still fail with Boost program options enabled.
Where's the right place in the code base to put a convenience function for converting sets of scalars to a string as I am doing in EnvironmentOptions for default management? |
Should I leave SequenceStatisticalOptions in place or remove it? Or at least remove its use in other Options classes? I see it's deprecated and it's complicating MetropolisHastingsSGOptions among other Options classes... |
Related, should other deprecated Options classes like MetropolisHastingsSGOptions be left in place or removed for this refactor? |
Refactor in support of libqueso#569 to enable various layered phases of constructing SipOptionsValues. This mirrors work done in EnvOptionsValues. Also minor cleanup in EnvOptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing MhOptionsValues. This mirrors work done in EnvOptionsValues.
Miscellaneous should be fine. SequenceStatisticalOptions was deprecated in the 0.55, 0.56, and 0.57 releases, so IMHO it's ready to remove. I'd do that in a separate PR though; we ought to be able to merge it quickly. Likewise for MetropolisHastingsSGOptions, or anything else that's been deprecated for a couple releases. |
Partially addresses libqueso#569 Following the lead from GPMSAOptions, refactor EnvOptionsValues to allow construction from defaults vs. passed options object, with optional override from parsed user input file. Finer grained construct, set_defaults(), parse(), further allow a client to intersperse setting options via C++ as well since member variables are public. This commit changes a couple historical behaviors: * If there was an inbound EnvOptionsValues object, the input file never got parsed. * An Environment could have a NULL m_optionsObj; now we construct one with default values. This could lead to subsequent code cleanup. Retains optional boost::program_options parser for now.
Partially addresses libqueso#569 by addressing feedback in PR libqueso#640, specifically: * Keep comments in header * Prefer reference to pointer * Cleanup m_optionsObj tests for NULL (adding require !NULL in a couple places) * Only initialize BoostInputOptionsParser when needed * Take care not to have trailing whitespace when converting set of integer to string Some tests still fail with Boost program options enabled.
Refactor in support of libqueso#569 to enable various layered phases of constructing SipOptionsValues. This mirrors work done in EnvOptionsValues. Also minor cleanup in EnvOptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing MhOptionsValues. This mirrors work done in EnvOptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing SfpOptionsValues. This mirrors work done in other *OptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing McOptionsValues. This mirrors work done in other *OptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing OptimizerOptions, MLSamplingLeveloptions, and InfiniteDimensionalMCMCSamplerOptions. This mirrors work done in other *OptionsValues.
To close out this issue, need to take one more cleanup pass and review:
|
Partially addresses libqueso#569 Following the lead from GPMSAOptions, refactor EnvOptionsValues to allow construction from defaults vs. passed options object, with optional override from parsed user input file. Finer grained construct, set_defaults(), parse(), further allow a client to intersperse setting options via C++ as well since member variables are public. This commit changes a couple historical behaviors: * If there was an inbound EnvOptionsValues object, the input file never got parsed. * An Environment could have a NULL m_optionsObj; now we construct one with default values. This could lead to subsequent code cleanup. Retains optional boost::program_options parser for now.
Partially addresses libqueso#569 by addressing feedback in PR libqueso#640, specifically: * Keep comments in header * Prefer reference to pointer * Cleanup m_optionsObj tests for NULL (adding require !NULL in a couple places) * Only initialize BoostInputOptionsParser when needed * Take care not to have trailing whitespace when converting set of integer to string Some tests still fail with Boost program options enabled.
Refactor in support of libqueso#569 to enable various layered phases of constructing SipOptionsValues. This mirrors work done in EnvOptionsValues. Also minor cleanup in EnvOptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing MhOptionsValues. This mirrors work done in EnvOptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing SfpOptionsValues. This mirrors work done in other *OptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing McOptionsValues. This mirrors work done in other *OptionsValues.
Refactor in support of libqueso#569 to enable various layered phases of constructing OptimizerOptions, MLSamplingLeveloptions, and InfiniteDimensionalMCMCSamplerOptions. This mirrors work done in other *OptionsValues.
Finish up libqueso#569: * Add default ctor to all Options classes and always init defaults and m_env * Always initialize BoostInputOptionsParser since operator<< may dereference it (there remains a bug w.r.t. same in copy ctors, but not going to take time on it as no current use cases and code is deprecated)
Enhance other (Environment, Metropolis Hastings, inverse problem) constructors with options setting flexibility similar to GPMSA, so one can default construct, set via C++ API, or parse from input file in flexible order. In particular, it's not possible to set through C++, then override with power user options from an input file.
See discussion with @roystgnr https://groups.google.com/forum/#!topic/queso-users/S58W-IzRRtI
Related, there is a potential chicken and egg problem with the options for constructing an Environment, as parsing Environment options from a file requires an Environment...
The text was updated successfully, but these errors were encountered: