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

Allow user to override library option defaults #1773

Merged
merged 9 commits into from
Nov 22, 2019
Merged

Conversation

johnomotani
Copy link
Contributor

In STORM we've started doing something like this to change the defaults for options (e.g. boundary conditions) that should always be set a certain way for a certain physics model, so that we don't have to have so many things in the input file

opt["n"]["bndry_xin"] = opt["n"]["bndry_xin"].withDefault("neumann_o2");

This PR is intended to allow this to be done for more options, by adding a static method PhysicsModel::setDefaults() that user physics model can override. This method is called in BOUTMAIN just after the options are read, and before Mesh, etc. are created. Also adds a convenience method for overriding defaults. So with this PR we can do something like

class MyPhysicsModel {
  static void setDefaults() {
    auto& options = Options::root();
    options["mesh"]["staggergrids"].overrideDefault(true);
  }
};

to make mesh:staggergrids default to true rather than false.

Comments on the design very welcome!

Note: it is important to use a reference auto& options rather than a copy so the options that are changed are changed in the global options. I thought about removing the copy constructor from Options so auto options = Options::root() would be an error and prevent this mistake - it seems to me like it would be nice but unfortunately std::map uses std::pair and std::pair requires a copy constructor, although this might change in C++17 https://stackoverflow.com/questions/23723744/stdpair-too-restrictive-constructor

@johnomotani johnomotani added feature proposal A code/feature outline proposal small-change Changes less than 100 lines - should be quick to review labels Jul 20, 2019
@johnomotani johnomotani added this to the BOUT-4.3 milestone Oct 23, 2019
@ZedThree
Copy link
Member

Looks useful but I'm not terribly comfortable adding functionality to PhysicsModel in 4.3.0. Happy for it to go into next though, where we'll have more time to iterate on it if needed!

@johnomotani johnomotani removed this from the BOUT-4.3 milestone Oct 23, 2019
@johnomotani johnomotani changed the base branch from v4.3.0-rc to next October 23, 2019 15:14
@johnomotani
Copy link
Contributor Author

I wonder if there's an alternative design for this kind of feature, where the user could put some code to 'register' a default for an option, a bit like REGISTER_DERIVATIVE. If there's a way of doing this it would have the advantage of being more specific than the setDefaults() method proposed here, where the user could insert arbitrary code into BOUT++'s initialisation.

@ZedThree
Copy link
Member

Ah, yes! Can probably be done if overrideDefault returned a value:

// Inside anonymous namespace because we don't want it to be a real global value
namespace {
const auto mydefault = Options::root()["mesh"]["staggergrids"].overrideDefault(true);
}

Putting that outside of main should be sufficient as it would initialise mydefault before calling main and I believe Options doesn't need anything else set up first (could be wrong). Could be wrapped up in a macro to make it easier to spell, and to take care of making a unique name for it:

#define BOUT_USER_OVERRIDE_DEFAULT_OPTION(option, value) \
  namespace { const auto user_default ## __FILE__ ## __LINE__ = \
    Options::root() option .overrideDefault(value) }

BOUT_USER_OVERRIDE_DEFAULT_OPTION(["mesh"]["staggergrids"], true)

(note: completely untested and also horrible)

The gross thing there is the option has to be passed in as indexing, which would be super easy to break. Could be done if we could make Options handle ["mesh:staggergrids"] as well. Not sure if that's a good idea or not.

@johnomotani
Copy link
Contributor Author

Making Options handle ["mesh:staggergrids"] turned out to be straightforward, only 4 lines of code in two places, so that seems like a good way to go to me (unit test included).

I've added a macro as @ZedThree suggested to override defaults, and reverted the changes to BoutInitialise and PhysicsModel. If the new version looks good, I think it would be good to rebase this PR to get rid of any commits touching bout.hxx/bout.cxx/physicsmodel.hxx. Also to remove the change to make.config.in, which is a change that is in master, but I'd added by mistake to the first commit of this PR.

I've also added the "mesh:staggered" support, and BOUT_OVERRIDE_DEFAULT_OPTION to the manual.

Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

Looks good! Please could you also add a unit test for overrideDefault?

ASSERT_TRUE(options.isSection(""));
ASSERT_FALSE(options.isSection("subsection"));

options["subsection:testkey"] = 1.;
Copy link
Member

Choose a reason for hiding this comment

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

Please could you change "testkey" and 1. here? I get nervous when test values could potentially clash!

@@ -473,6 +473,23 @@ public:
return val;
}

/// Allow the user to override defaults set later, intended to be called in
/// methods overriding PhysicsModel::setDefaults.
Copy link
Member

Choose a reason for hiding this comment

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

Comment is out of date

Allows user to set a custom default for an option, as long as the method
is called before the option is fetched using withDefault(). Will support
a macro to allow users to override any library defaults.
The value of the option will be output where it is used, so output from
overrideDefault is just duplication.
The overrideDefault method should not be used to get the value of an
option - it does not make sense to do so because there is already the
withDefault method. So there is no need to return a value from the
method - removing the return reduces the number of lines in the method
and avoids potential confusion about where it should be used.

Also do not set value_used=true in overrideDefault, since it is possible
that the value might never be used (unlike in withDefault).
Allow sections to be given in the string, separated with ":", e.g.
Options::root()["section:subsection:myopt"]
Allows global variable to be created so that overrideDefault() can be
called before main(), allowing the user to change library default
options before they are used.
BOUT_OVERRIDE_DEFAULT_OPTION(name, value) can be called in global
namespace to override the default for the option "name".
Prevents errors from calls like
options["somekey"].overrideDefault("string_value");
@johnomotani johnomotani added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label Nov 4, 2019
@ZedThree ZedThree merged commit f0297b0 into next Nov 22, 2019
@ZedThree ZedThree deleted the user-option-defaults branch November 22, 2019 16:39
@johnomotani johnomotani restored the user-option-defaults branch November 22, 2019 18:14
@ZedThree ZedThree deleted the user-option-defaults branch December 2, 2019 17:00
johnomotani referenced this pull request Mar 31, 2020
Previously communicateXZ() actually communicated in the y-direction as
well, the only difference with communicate() was that it did not apply
the twist-shift. Since sendX() method was added, can now actually
change the implementation to communicate only in X.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 feature proposal A code/feature outline proposal small-change Changes less than 100 lines - should be quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants