Stan refactor #2004
Merged
Stan refactor #2004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
On Dec 28, 2016, at 11:21 PM, Daniel Lee ***@***.***> wrote:
@syclik commented on this pull request.
In src/stan/services/optimize/bfgs.hpp:
> + model.write_array(rng, cont_vector, disc_vector, values,
+ true, true, &msg);
+ if (msg.str().length() > 0)
+ message_writer(msg.str());
+
+ values.insert(values.begin(), lp);
+ parameter_writer(values);
+ }
+ int ret = 0;
+
+ while (ret == 0) {
+ interrupt();
+ if (refresh > 0
+ && (bfgs.iter_num() == 0
+ || (bfgs.iter_num() + 1 % refresh == 0)))
+ message_writer(" Iter "
No. Of course not. But I'm trying hard not to fix everything that's wrong with our whole code base or we'll never get this in. This has been in since it was written -- it bothers me too, but I'm really trying to fix the things that need to be fixed. We can walk through the code base later.
Isn't the behavior going to be wrong here given that % binds
more tightly than +. If so, could you please create an issue
for this? Thanks. I can't actually understand the logic or I'd
do it myself.
|
|
This one hurts. It's in ADVI and it's propagated through to CmdStan. I don't think all our developers are on board with the terminology.
Michael's OK with it, and I think we can just enforce it.
But I'm OK taking a pass through this later. We can't actually
remove the existing naming until Stan 3, but we could replace
it and deprecate the old behavior.
Sorry if the comments are overwhelming. I'm just noting
everything that pops out at me reviewing the code.
I don't think it's hopeless, but I think fixing this should come with fixing all of ADVI. I'm going to create a new issue for all the things you've written up.
Thanks!
+ * @param[in] tol_rel_obj argument to validate
We haven't been too consistent with this. I can either add them everywhere or remove them. Or just let it be haphazard until we know what we want.
OK. It's very inconsistent in the code base. I often
forget because it wasn't in Javadoc when I used it (or I
didn't know about it).
As to what we want, we can have the `[in]` everywhere if
you think it's helpful to appear more consistent. It's
exactly like adding `const` to a primitive---it doesn't do
anything to the external API because C++ pass by value.
That is, it has to be `[in]` by construction if it's a primitive
passed by value, the same way it's `const` by construction.
In src/stan/services/experimental/advi/fullrank.hpp:
> +
+ stan::variational::advi<Model,
+ stan::variational::normal_fullrank,
+ boost::ecuyer1988>
+ cmd_advi(model,
+ cont_params,
+ rng,
+ grad_samples,
+ elbo_samples,
+ eval_elbo,
+ output_samples);
+ cmd_advi.run(eta, adapt_engaged, adapt_iterations,
+ tol_rel_obj, max_iterations,
+ message_writer, parameter_writer, diagnostic_writer);
+
+ return 0;
It can throw. ADVI is a mess, so I can't really track through all the different types of things it can or can't do, so I'm going to stick it on the ADVI issue I'm creating.
Thanks. I'm not too worried about ADVI until it's a bit more
robust and we can recommend it to users in some situations.
|
|
sorry -- I misread the original comment. It does happen in a lot of places. Let me see how many spots and get this going if it's not too disruptive.
Or just create a new issue so we don't forget. I really don't want to drag this one out---I was just commenting on all the stuff that I saw.
|
|
From CmdStan, RStan, PyStan. I'm pretty sure that's what we decided. Personally, I would have wanted to have (redundant) validation inside the service functions, but I think at some point I was convinced that it was better practice to validate arguments outside.
We wanted to do this outside so that the interfaces could produce error messages in their own language.
|
|
Yup. That's right. I think the current implementation isn't perfect, but
it'll work for now.
…On Fri, Dec 30, 2016 at 12:12 PM, Bob Carpenter ***@***.***> wrote:
> From CmdStan, RStan, PyStan. I'm pretty sure that's what we decided.
Personally, I would have wanted to have (redundant) validation inside the
service functions, but I think at some point I was convinced that it was
better practice to validate arguments outside.
We wanted to do this outside so that the interfaces could produce error
messages in their own language.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2004 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_F_lMbEafmm7KkGEDycBXfnwB0r-vks5rNTuSgaJpZM4Jfpda>
.
|
I think one source of error messages (via throwing exceptions) would be fine. I thought there were other reasons pushing validation outside the function. |
|
Bob, action is back on you to review the pull request. I made most of the changes you suggested. If I missed anything major, please remind me. The ADVI issues, I've created a new issue: #2195. |
|
@syclik Thanks! That's awesome. I'll be able to take another pass tomorrow (1 January 2017!). |
bob-carpenter
approved these changes
Jan 3, 2017
| * | ||
| * @param[in] state Values in a std::vector | ||
| */ | ||
| void operator()(const std::vector<double>& state) { |
There was a problem hiding this comment.
this is 100% code dup with previous---I would ratha single templated impl called twice
| * Output stream | ||
| */ | ||
| std::ostream& output_; | ||
| /** |
There was a problem hiding this comment.
space between declarations
| /** | ||
| * Virtual destructor. | ||
| */ | ||
| virtual ~writer() {} |
There was a problem hiding this comment.
move dtor up before other methods
| */ | ||
| std::vector<double> vals_r(const std::string& name) const { | ||
| std::vector<double> vals_r; | ||
| return vals_r; |
There was a problem hiding this comment.
don't need the assign
| /** | ||
| * Destructor. | ||
| */ | ||
| ~random_var_context() {} |
| callbacks::writer& diagnostic_writer) { | ||
| util::experimental_message(message_writer); | ||
|
|
||
| boost::ecuyer1988 rng = util::rng(random_seed, chain); |
There was a problem hiding this comment.
shiuldnt this all be templated on RNG for consistency with all else
| parameter_writer(names); | ||
|
|
||
| Eigen::VectorXd cont_params; | ||
| cont_params.resize(cont_vector.size()); |
There was a problem hiding this comment.
make change i suggested here
| model.constrained_param_names(names, true, true); | ||
| parameter_writer(names); | ||
|
|
||
| Eigen::VectorXd cont_params; |
| break; | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
these braces don't do anything
| "------------------------------"); | ||
| message_writer("EXPERIMENTAL ALGORITHM:"); | ||
| message_writer(" - please expect frequent updates to the procedure"); | ||
| message_writer(" - please expect unexpected inference results"); |
There was a problem hiding this comment.
expect the unexpected? how about instead of both lines:
This procedure has not been thoroughly tested and may be unstable or buggy. The interface is subject to change.
|
This pull request was too large---it overwhelmed my mac and ipad trying to review. I lost about ten comments, mostly about all the repeated code snippets that should be oulled into functions. Never mind. I just approved it because as much as I hate to see all this code duplication, our code doesnt need to be perfectly factored going forward and I want to be pragmatic and get this in ASAP |
|
On Jan 3, 2017, at 3:48 PM, Daniel Lee ***@***.***> wrote:
@syclik commented on this pull request.
...
+ Eigen::VectorXd cont_params;
+ cont_params.resize(cont_vector.size());
I'm changing both instances to construct with the right size. We can't use Eigen::Map -- it doesn't compile.
It compiles and passes tests on my machine with both assignment:
```
Eigen::VectorXd cont_params
= Eigen::Map<Eigen::VectorXd>(&cont_vector[0], cont_vector.size(), 1);
```
and construction:
```
Eigen::VectorXd cont_params(Eigen::Map<Eigen::VectorXd>(&cont_vector[0], cont_vector.size(), 1));
```
Not sure it's faster or clearer, though. Here's the lowdown on Map:
https://eigen.tuxfamily.org/dox/group__TutorialMapClass.html
- Bob
|
|
Just communicated with @ariddell and he gave it a thumbs up. This is now ready to merge as soon as the final tests pass. We have:
As soon as the tests pass, I'm merging. |
|
I think it is fine.
…On Sun, Jan 8, 2017 at 9:45 PM, Daniel Lee ***@***.***> wrote:
Just communicated with @ariddell <https://github.com/ariddell> and he
gave it a thumbs up.
This is now ready to merge as soon as the final tests pass. We have:
- a review of the Stan branch by @bob-carpenter
<https://github.com/bob-carpenter>
- sign-off by a CmdStan maintainer, me
- sign-off by an RStan maintainer, @bgoodri
<https://github.com/bgoodri>
- sign-off by the PyStan maintainer, @ariddell
<https://github.com/ariddell>
As soon as the tests pass, I'm merging.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2004 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqgsIfyyL9M8hE-erL1OXzo9_Na8jks5rQZ8-gaJpZM4Jfpda>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Submisison Checklist
./runTests.py src/test/unitmake cpplintSummary
This pull request accomplishes two major things:
src/stan/servicesfolder (thestan::servicesnamespace), we now have functions that should be called by the interfaces.It does this while maintaining all current behavior.
Intended Effect
The intended effect is to make the downstream interfaces easier to implement and maintain.
The code is much cleaner. Each of the functions in
src/stan/servicesexcluding thesrc/stan/services/utilfolder are meant to be called by RStan, PyStan, and CmdStan. This will also make it much easier to deal with the upcoming template build issue we will have with the introduction of forward-mode and RHMC.How to Verify
(I'm sorry -- the diff is large.)
developbranch not having changes necessary, but I'll submit a pull request on the CmdStan branch.)src/stan/services. I've covered all the use cases of CmdStan, PyStan, and RStan. See side effects below.src/stan/servicesthat were added. Take a look.If there are some oddly implemented things in there, most weird implementations are due to keeping the existing behavior intact. This way, we can roll these changes out with a minor release and keep moving forward.
Side Effects
CmdStan, RStan, and PyStan will break under the current branch, but will maintain the same behavior.
I have a branch of CmdStan that works. Allen has a branch of PyStan that's almost working. I have a branch of RStan that's almost working.
Documentation
The C++ code may need some more doc.
If anyone wants to help, there's that and adding more tests.
Reviewer Suggestions
This one's big, so it'd be helpful to have a few eyes on it.
@ariddell, @bgoodri: since this will effect both of you directly.
@sakrejda: since you've been looking at this for a while.
@bob-carpenter, @betanalpha, @rtrangucci: if you've got the time.
Although I'm confident that this behaves identically, this one shouldn't be taken lightly.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: