Improve tuning by skipping the first samples + add new experimental tuning method #5004
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
twiecki
reviewed
Sep 20, 2021
| @@ -342,6 +360,8 @@ def __init__( | |||
|
|
|||
| def add_sample(self, x, weight): | |||
| x = np.asarray(x) | |||
| if weight != 1: | |||
| raise ValueError("weight is unused and broken") | |||
There was a problem hiding this comment.
Suggested change
| raise ValueError("weight is unused and broken") | |
| raise ValueError("Setting weight != 1 is not supported.") |
Or maybe we should just remove it all-together.
twiecki
reviewed
Sep 20, 2021
pymc3/tests/test_sampling.py
Outdated
| @@ -87,7 +87,7 @@ def test_sample(self): | |||
|
|
|||
| def test_sample_init(self): | |||
| with self.model: | |||
| for init in ("advi", "advi_map", "map"): | |||
| for init in ("advi", "advi_map", "map", "jitter+adapt_diag_grad"): | |||
There was a problem hiding this comment.
Should we add all the others here too?
twiecki
changed the title
|
Needs a line in the release-notes. |
This comment has been minimized.
This comment has been minimized.
|
I noticed something similar when debugging |
This was referenced Sep 20, 2021
This comment has been minimized.
This comment has been minimized.
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
reviewed
Sep 20, 2021
Comment on lines
361
to
364
| def add_sample(self, x, weight): | ||
| x = np.asarray(x) | ||
| if weight != 1: | ||
| raise ValueError("Setting weight != 1 is not supported.") |
There was a problem hiding this comment.
Suggested change
| def add_sample(self, x, weight): | |
| x = np.asarray(x) | |
| if weight != 1: | |
| raise ValueError("Setting weight != 1 is not supported.") | |
| def add_sample(self, x, weight=None): | |
| if weight is not None: | |
| warning.warn( | |
| "Setting weight is no longer supported and and will raise an error in the future.", | |
| DeprecationWarning, | |
| ) | |
| x = np.asarray(x) |
There was a problem hiding this comment.
I think a hard break is fine here. This really was internal, unused and wrong
There was a problem hiding this comment.
Then I would suggest removing the weight argument altogether
ricardoV94
reviewed
Sep 20, 2021
ricardoV94
approved these changes
Sep 22, 2021
|
Thanks @aseyboldt |
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.
Tuning the mass matrix starts right away in the current implementation, but in most models during the first couple of samples we are only moving to the typical set, so we do not get information about the posterior variance at all. In the worst case we learn a mass matrix that doesn't match the posterior at all, so that sampling the the first adaptation window will be very slow (you can see this a slowdown of sampling after step 100). Usually, we will recover from this, but it seems to be better to just skip those samples during adaptation in the first place.
In an example model by @ricardoV94 we can see this behavior clearly when we look at the distance of the currently used mass matrix to the final mass matrix:
This PR also contains an experimental tuning implementation using gradients and samples that can be enabled by
init="jitter+adapt_diag_grad". During tests on a few models this seems to be more stable than the only sample based tuning system we use right now, but there are also a few cases where it performs worse. For posteriors that are normal it should converge to the same mass matrix as our current implementation (and much faster), but for non-normal posteriors the result can differ. Unfortunately I don't know of any other way to tell which is better other than trying it on a large number of models.An example notebook can be found here:
https://gist.github.com/aseyboldt/7897fbddacacaa0c86efc917afe9ce3f