Config validation, request for feedback

Developers
1

I’d like some feedback on an idea for argument validation in the github.com:sakrejda/stan-config repo. There’s a BUILD file in the repo with instructions that “should” work with cmake (haven’t had lots of computers to test it). The repo has classes for some arguments and tests.

I was playing around with templates for simplifying argument validation and I was wondering if this might make it manageable to put a mechanism into cmdstan or stan for argument validation without adding a lot of code. In general you need a per-arg class like CmdStan currently uses, but you pass some template parameters to identify what the argument should be, so in this line:

  class num_samples : stan::config::argument<int, num_samples, integer, positive> {

int should really be taken from num_samples::from but I’m having some issues with incomplete types.
num_samples is for CRTP, integerandpositive` say the argument will throw on construction if it’s not a positive integer.

So basically the logic is move mostly out of the class. Here’s an example class.

#include <stan/config/argument.hpp>
#include <stan/config/argument_traits.hpp>

namespace stan {
  namespace config {

    class num_samples : stan::config::argument<int, num_samples, integer, positive> {
      public:
        num_samples();
        num_samples(int arg);
        typedef int from;
        const static std::string description;
        const static int default_value;
    };

    num_samples::num_samples() : num_samples::num_samples(num_samples::default_value) {}
    num_samples::num_samples(int arg) : stan::config::argument<int, num_samples, integer, positive>(arg) {}

    const std::string num_samples::description = "Integer (n) describing the number of "
      "samples to generate from the posterior density.  Includes warmup iterations. ";
    const int num_samples::default_value = 1000;

  }
}

Would this be worth using to simplify argument validation in CmdStan or more broadly?

2

Just look at the old argument objects. They already implemented all of these features, including constraints with valid and invalid values so that each argument and even entire argument configurations can be tested automatically.

3

What’s your point? I know that there was a proposal to include argument
validation but it didn’t make it into the code base. The question is
whether this is simple enough to make it into the code base.

4

No, it was in the code doing all of the validation and automated testing (including all possible combinatoric configurations) before the refactor. You’re recreating what Dan took out.

5

Nothing was taken out. It was just moved to CmdStan.

See: argument_configuration_test.cpp in CmdStan.

@sakrejda: I haven’t taken a look at your repo yet… those were just factual statements. I’ll get you feedback when I have a little more free time.

1 Like
6

I see what you mean from @syclik’s link. My goal is a little different: something to check the configuration passed in by the user is sane so we don’t have code scattered across all the interface repos to do it. I’m not claiming the code in that repo does it all, I’m claiming it’s an approach that could do it with minimal code over what’s currently in CmdStan.

7

That is literally what the code did/does. See, for example, https://github.com/stan-dev/cmdstan/blob/develop/src/cmdstan/arguments/arg_stepsize.hpp. It’s almost exactly the same as what you are suggesting here with the arguments split into their type so that they can be given appropriate values.

8

Ouch, I missed the bits of code that actually do validation. I guess the functionality added is fairly minimal then.