-
Notifications
You must be signed in to change notification settings - Fork 39
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
Refactor logit transform logic into TK #556
Conversation
The transform logic arguably belongs in the TK.
This transform logic happens when calling updateCovMatrix on the TK to feed it a new cov matrix after Adaptive Metropolis. Since the sequence generator holds a reference to the base class, I had to add a pure virtual updateCovMatrix method in the base class. Derived classes that don't have covariance matrices or don't support adaptivity will throw an exception in this method.
* If the TK support delayed rejection, derived classes should scale by a | ||
* factor of \f$ 1/scales^2 \f$. | ||
*/ | ||
virtual void updateLawCovMatrix(const M & covMatrix) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, this change breaks backwards compatibility. In my defence, I highly doubt anybody is subclassing TKs yet; the factory infrastructure is quite new.
Instead, I can make this function virtual but non-pure and throw a run-time exception instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd definitely prefer the compile-time failure option to the run-time failure option...
This PR serves as a first step in addressing #472. |
Codecov Report
@@ Coverage Diff @@
## dev #556 +/- ##
==========================================
- Coverage 69.44% 69.43% -0.02%
==========================================
Files 170 170
Lines 14998 14998
==========================================
- Hits 10416 10414 -2
- Misses 4582 4584 +2
Continue to review full report at Codecov.
|
👍 |
The fact we were doing logit transforms outside of the TK designed for logit transforms bothered me.
I can't do it everywhere, though, one of the pieces of logic relies fundamentally on the adaptivity formulation. See #555 for details.