Sure. We also already merged a similar, but not the same, commit into develop (and this is where we differ from how people normally use that git flow). I would want to be very careful when trying to merge master back into develop and replaying similar commits with similar intentions on top of each other - I think you’re right that if git finds the line already matches what it was supposed to become then there won’t be a change and it will work out fine. Issues only crop up if other people have edited those lines in the meantime, and we’ve had a lot of files touched already on develop (like putting all tests under cpplint). So that’s why I think your route implies more work and caution. But you mentioned a reason why having a tag that isn’t merged into master doesn’t work well for RStan and I was hoping you could dive more into that - I would have imagined RStan 2.17.1 would point at Stan tag v2.17.1, no?
If you do
git checkout bugfix/revert_string
git merge -s recursive -Xtheirs --no-commit develop
there are no merge conflicts, although one test got broken. After fixing that test, there will be no conflicts if you merge bugfix/revert_string into develop. After doing that, there shouldn’t be any future problems merging develop into master. Agit mergedoesn't replay all the intervening commits like subversion orgit rebase, so it only matters what is in the tip ofdevelopwhen it is merged intomaster`. If, for whatever reason — which I can’t and I haven’t heard anyone else thing of one — that is not fine, then
git checkout master
git merge --no-commit develop
will show what the problem is and
git reset --hard HEAD
will put develop back into its state when bugfix/revert_string was merged into it. At that point, we can fix develop however we need to before trying to merge it into master again.
The StanHeaders repo typically points to the master branches of its two submodules, although that is ordinarily the latest tagged commit anyway. The StanHeaders R package that goes on to CRAN is a bit different to avoid breaking some other R packages.
I don’t trust -Xtheirs - it broke one test that you detected, for example. I think we’d have to manually inspect all of the merge conflicts.
OK but that is tantamount to saying you don’t trust our tests either.
I disagree - these conflicts alter the tests from the state in which I trust them.
You don’t have to trust that the tests compile (which they don’t in one case). You just have to trust that when the tests compile and pass, the code is not in a worse shape than it was when it passed previously.
Right - I don’t trust that if there are uninspected changes to the tests.
git checkout revert_string
git merge -s recursive -Xtheirs --no-commit develop
git diff --ignore-space-change develop test/
Unfortunately, I can’t convince git to ignore added newlines at the end of files, but here they are:
diff --git a/test/unit/math/prim/mat/err/check_greater_or_equal_test.cpp b/test/unit/math/prim/mat/err/check_greater_or_equal_test.cpp
index 7f3f0b9594..bbac02e37f 100644
--- a/test/unit/math/prim/mat/err/check_greater_or_equal_test.cpp
+++ b/test/unit/math/prim/mat/err/check_greater_or_equal_test.cpp
@@ -220,3 +220,4 @@ TEST(ErrorHandlingScalar, CheckGreaterOrEqual_nan) {
}
}
}
+
diff --git a/test/unit/math/prim/mat/err/check_greater_test.cpp b/test/unit/math/prim/mat/err/check_greater_test.cpp
index 5528a85b89..7a952182f9 100644
--- a/test/unit/math/prim/mat/err/check_greater_test.cpp
+++ b/test/unit/math/prim/mat/err/check_greater_test.cpp
@@ -181,3 +181,4 @@ TEST(ErrorHandlingScalar, CheckGreater_nan) {
EXPECT_THROW(check_greater(function, "x", nan, nan),
std::domain_error);
}
+
diff --git a/test/unit/math/prim/mat/err/check_less_or_equal_test.cpp b/test/unit/math/prim/mat/err/check_less_or_equal_test.cpp
index 689f085764..3ce01e99c8 100644
--- a/test/unit/math/prim/mat/err/check_less_or_equal_test.cpp
+++ b/test/unit/math/prim/mat/err/check_less_or_equal_test.cpp
@@ -184,3 +184,4 @@ TEST(ErrorHandlingScalar, CheckLessOrEqual_nan) {
}
}
}
+
diff --git a/test/unit/math/prim/mat/err/check_pos_definite_test.cpp b/test/unit/math/prim/mat/err/check_pos_definite_test.cpp
index c93fe81f55..7eda6b3958 100644
--- a/test/unit/math/prim/mat/err/check_pos_definite_test.cpp
+++ b/test/unit/math/prim/mat/err/check_pos_definite_test.cpp
@@ -246,3 +246,4 @@ TEST_F(ErrorHandlingMatrix, checkPosDefinite_nan) {
std::domain_error,
expected_msg.str());
}
+
diff --git a/test/unit/math/prim/scal/err/check_2F1_converges_test.cpp b/test/unit/math/prim/scal/err/check_2F1_converges_test.cpp
index cba19861d8..e8f7a0903d 100644
--- a/test/unit/math/prim/scal/err/check_2F1_converges_test.cpp
+++ b/test/unit/math/prim/scal/err/check_2F1_converges_test.cpp
@@ -87,3 +87,5 @@ TEST(passesOnConvergentArgs, Check2F1Converges) {
EXPECT_NO_THROW(check_2F1_converges(function, a1, a2, b1, z));
EXPECT_NO_THROW(check_2F1_converges(function, a1, a2, b1, z));
}
+
+
diff --git a/test/unit/math/prim/scal/err/check_3F2_converges_test.cpp b/test/unit/math/prim/scal/err/check_3F2_converges_test.cpp
index 0985b5f8d7..2eda12d7fd 100644
--- a/test/unit/math/prim/scal/err/check_3F2_converges_test.cpp
+++ b/test/unit/math/prim/scal/err/check_3F2_converges_test.cpp
@@ -116,3 +116,5 @@ TEST(passesOnConvergentArgs, Check3F2Converges) {
EXPECT_NO_THROW(check_3F2_converges(function, a1, a2, a3, b1, b2, z));
EXPECT_NO_THROW(check_3F2_converges(function, a1, a2, a3, b1, b2, z));
}
+
+
diff --git a/test/unit/math/prim/scal/err/check_greater_or_equal_test.cpp b/test/unit/math/prim/scal/err/check_greater_or_equal_test.cpp
index 7e6d7dd8c2..22c27fa957 100644
--- a/test/unit/math/prim/scal/err/check_greater_or_equal_test.cpp
+++ b/test/unit/math/prim/scal/err/check_greater_or_equal_test.cpp
@@ -51,3 +51,4 @@ TEST(ErrorHandlingScalar, CheckGreaterOrEqual_nan) {
EXPECT_THROW(check_greater_or_equal(function, "x", nan, nan),
std::domain_error);
}
+
diff --git a/test/unit/math/prim/scal/err/check_greater_test.cpp b/test/unit/math/prim/scal/err/check_greater_test.cpp
index 976055d038..90d2ae2fa4 100644
--- a/test/unit/math/prim/scal/err/check_greater_test.cpp
+++ b/test/unit/math/prim/scal/err/check_greater_test.cpp
@@ -48,3 +48,4 @@ TEST(ErrorHandlingScalar, CheckGreater_nan) {
EXPECT_THROW(check_greater(function, "x", nan, nan),
std::domain_error);
}
+
diff --git a/test/unit/math/prim/scal/err/check_less_or_equal_test.cpp b/test/unit/math/prim/scal/err/check_less_or_equal_test.cpp
index 5a966388b0..fbf38d6a8c 100644
--- a/test/unit/math/prim/scal/err/check_less_or_equal_test.cpp
+++ b/test/unit/math/prim/scal/err/check_less_or_equal_test.cpp
@@ -51,3 +51,4 @@ TEST(ErrorHandlingScalar, CheckLessOrEqual_nan) {
EXPECT_THROW(check_less_or_equal(function, "x", nan, nan),
std::domain_error);
}
+
diff --git a/test/unit/math/rev/scal/err/check_less_or_equal_test.cpp b/test/unit/math/rev/scal/err/check_less_or_equal_test.cpp
index b2208b0b95..27852a3d10 100644
--- a/test/unit/math/rev/scal/err/check_less_or_equal_test.cpp
+++ b/test/unit/math/rev/scal/err/check_less_or_equal_test.cpp
@@ -65,3 +65,4 @@ TEST(AgradRevErrorHandlingScalar, CheckLessOrEqualVarCheckUnivariate) {
stan::math::recover_memory();
}
+
diff --git a/test/unit/math/rev/scal/err/check_not_nan_test.cpp b/test/unit/math/rev/scal/err/check_not_nan_test.cpp
index 3db6717f64..6877f1cc2b 100644
--- a/test/unit/math/rev/scal/err/check_not_nan_test.cpp
+++ b/test/unit/math/rev/scal/err/check_not_nan_test.cpp
@@ -73,3 +73,4 @@ TEST(ErrorHandlingScalar, CheckNotNanVarCheckUnivariate) {
stan::math::recover_memory();
}
+
Those look like pretty innocuous changes. How does StanHeaders get released? Is it like a script that a person fires off on demand and it right now is programmed to always pull down Stan’s master?
In principle, I just go into the StanHeaders folder of the rstan repo, update the submodules to master, and execute
R CMD build StanHeaders # makes StanHeaders_2.17.1.tar.gz
R CMD check StanHeaders_2.17.1.tar.gz
upload to https://win-builder.r-project.org/ to see if it works on Windows and then upload to https://xmpalantir.wu.ac.at/cransubmit/ .
In practice, it there are additional steps:
mv StanHeaders_2.17.1.tar.gz /tmp
cd /tmp
tar -xzf StanHeaders_2.17.1.tar.gz
# Fix broken things
R CMD build StanHeaders
R CMD check StanHeaders_2.17.1.tar.gz
and upload to https://xmpalantir.wu.ac.at/cransubmit/ .
Gotcha. Would it be hard to just update the submodules to a v2.17.1 tag instead of master? I have a branch with the full test suite run against just the relevant commits to fix the performance (and make the tests pass) here: https://github.com/stan-dev/math/pull/674
I wouldn’t merge that in anywhere; instead I’d just tag it and make a release based on it in github. The release scripts also rely on master in quite a few places so it’d be a little manual.
I still have this fear based on ignorance and related but different bad past experiences with cherry-picking and that kind of thing that makes me prefer this, but if you’re pretty sure about merging this branch into master I’d go ahead and do it. Any tests you recommend running on the branch that can lend me confidence?
Is there an expected timeline when the new version of StanHeaders and rstan will be submitted to CRAN?
I think we are going to discuss it today.
Is there any news on the question when we can expect the next version?
Sorry to be so insistent about it, but I would really like to use Stan with an LKJ prior that has correct constants.
If I read the thread on the lkj bug correctly, it only applied to the rare scenario of using the lkj for random number generation. If it affected using the lkj as I prior, I presume there’d be much more panic to get the fix out since lkj is the recommended prior for models with correlations.
As far as I know it only affects the calculation of marginal likelihoods and Bayes factors (e.g., as implemented in our bridgesampling package) not estimation or random number generation. So it is true that it is not very common, but it is still unfortunate that the bug exists for such a long time in the version available on CRAN.
We discussed this briefly at the Stan meeting today. I’ll try to summarize where we’re at and what we want to accomplish.
Summary
Our current release, v2.17.0, is slow. But it can be compiled with a compiler that’s pre-C++11.
Current state of develop requires a C++11 capable compiler.
Two suggestions:
- Release v2.17.1 that fixes the slowness and does not require C++11.
- Release v2.18.0 that fixes the slowness and requires C++11.
The difficulty with releasing v2.17.1 is due to conflicts, which is no longer an issue since @bgoodri has fixed it on a branch.
Decision
- Are we releasing a v2.17.1 or v2.18.0?
@seantalts and @bgoodri, I think you two are the key stakeholders in the first decision. - When are we releasing it?
@seantalts, you’re the key stakeholder here.
@seantalts and @bgoodri, mind discussing it? (if it were my decision, I’d just move forward with v2.18.0 and keep development moving forward)
I think from my perspective I’m still waiting to hear back from Ben on my last post in this thread. I have a 2.17.1 branch we can tag as 2.17.1 and release for CmdStan immediately, but merging that branch into master causes me some slight concern that maybe Ben can ameliorate. I’m also happy to do a normal 2.18.0.
@bgoodri, can you respond to this comment from @seantalts?
@seantalts, I think to answer your first question: if we tag v2.17.1 and that’s our latest version, we should just have master point to it. (Unless I’m missing a deeper point, but master should just always point to our latest tag?)