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

Mixed input file and C++ API options handling #569

Open
briadam opened this issue Apr 21, 2017 · 9 comments
Open

Mixed input file and C++ API options handling #569

briadam opened this issue Apr 21, 2017 · 9 comments

Comments

@briadam
Copy link
Contributor

briadam commented Apr 21, 2017

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...

@dmcdougall
Copy link
Member

If the user passes an options object, the input file never gets parsed. Probably want to mess around with that logic.

@briadam
Copy link
Contributor Author

briadam commented Sep 15, 2017

@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.

@briadam
Copy link
Contributor Author

briadam commented Sep 20, 2017

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.

@roystgnr
Copy link
Member

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.

briadam added a commit to briadam/queso that referenced this issue Sep 25, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Sep 27, 2017
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.
@briadam
Copy link
Contributor Author

briadam commented Oct 3, 2017

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?
Edit: Never mind, I went with Miscellaneous.h/C

@briadam
Copy link
Contributor Author

briadam commented Oct 3, 2017

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...

@briadam
Copy link
Contributor Author

briadam commented Oct 3, 2017

Related, should other deprecated Options classes like MetropolisHastingsSGOptions be left in place or removed for this refactor?

briadam added a commit to briadam/queso that referenced this issue Oct 3, 2017
briadam added a commit to briadam/queso that referenced this issue Oct 3, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 3, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing MhOptionsValues.  This mirrors work done in
EnvOptionsValues.
@roystgnr
Copy link
Member

roystgnr commented Oct 4, 2017

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.

briadam added a commit to briadam/queso that referenced this issue Oct 12, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 12, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 12, 2017
briadam added a commit to briadam/queso that referenced this issue Oct 12, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 12, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing MhOptionsValues.  This mirrors work done in
EnvOptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 14, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing SfpOptionsValues.  This mirrors work done in
other *OptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 14, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing McOptionsValues.  This mirrors work done in
other *OptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 15, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing OptimizerOptions, MLSamplingLeveloptions, and
InfiniteDimensionalMCMCSamplerOptions.  This mirrors work done in
other *OptionsValues.
@briadam
Copy link
Contributor Author

briadam commented Oct 15, 2017

To close out this issue, need to take one more cleanup pass and review:

  • That default member values are only used in set_defaults. Also review for cases where registerOption was setting default, but set_defaults might not be.
  • Consider adding default ctor to all classes and call set_defaults()
  • Always initialize BPO parser in default constructors to avoid seg fault in ostream insertion
  • Make sure checkOptions() functions take no args
  • Make sure all ctors initialize m_env
  • Make sure all parse call set_prefix()

briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
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.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing MhOptionsValues.  This mirrors work done in
EnvOptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing SfpOptionsValues.  This mirrors work done in
other *OptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing McOptionsValues.  This mirrors work done in
other *OptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
Refactor in support of libqueso#569 to enable various layered
phases of constructing OptimizerOptions, MLSamplingLeveloptions, and
InfiniteDimensionalMCMCSamplerOptions.  This mirrors work done in
other *OptionsValues.
briadam added a commit to briadam/queso that referenced this issue Oct 16, 2017
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants