Modifying stan_csv_reader to fix a problem in CmdStan

Developers
1

This is in reference to the bug report about CmdStan doing the max. treedepth diagnosis incorrectly.

Currently, CmdStan hardcodes the maximum tree depth rather than reading it from the CSV file, which is not a big surprise, since the stan::io::stan_csv_reader class doesn’t read in that bit of information from the CSV file. What I’m thinking of doing is the following:

  • Modifying the stan::io::stan_csv_metadata struct to have an integer member max_depth for the maximum treedepth (and having the struct’s constructor initialize it to 10).

  • Modifying stan::io::stan_csv_reader::read_metadata to read in the max. tree depth. As far as I can tell, this should be as simple as adding else if (name.compare("max_depth") == 0) {metadata.max_depth = boost::lexical_cast<int>(value);} to the function.

Once that’s done, modifying CmdStan’s diagnose utility should be as straightforward as changing int max_limit = 10; to int max_limit = stan_csv.metadata.max_depth and fixing the instances where the variable max was used where max_limit should have been used.

I do have a couple questions, though.

First, is there any objection to my approach to modifying stan_csv_reader? Would it mess up any overarching design scheme for Stan readers or introduce any other problems?

Second, if my approach to fixing this CmdStan problem is reasonable, would I need to do two pull requests, one for the CSV reader (which as far as I can tell is part of the common Stan code rather than CmdStan itself) and one for CmdStan’s diagnose utility?

1 Like
2

That’s reasonable. Could you transfer this to an issue on stan and cmdstan? Or update the existing issue with an explanation of how it’ll be fixed. Thanks.

3

Sorry I haven’t gotten around to doing a pull request yet. One thing I’m still not sure of is what to do when a fix affects multiple Stan repositories, in this case stan and cmdstan. I’m still not sure if I need to make a separate pull request for each repository or not.

4

If the upstream fix to stan breaks cmdstan, then testing gets tricky. Otherwise, just change stan, wait until that gets merged, then change cmdstan.

We’re going to move math, stan and cmdstan into a single repo as soon as we find the cycles to do so to make this easier going forward.

5

looks like this change got made - thanks!