-
Notifications
You must be signed in to change notification settings - Fork 107
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
New integrator, and add some metadata to integrators.py #681
Merged
Merged
Changes from 54 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
b60e4ca
TESTS
reubenharry 0c5aa2d
TESTS
reubenharry 5eeb3e1
UPDATE DOCSTRING
reubenharry 4a09156
ADD STREAMING VERSION
reubenharry dfb5ee0
ADD PRECONDITIONING TO MCLMC
reubenharry 2ab3365
ADD PRECONDITIONING TO TUNING FOR MCLMC
reubenharry 4cc3971
UPDATE GITIGNORE
reubenharry f987da3
UPDATE GITIGNORE
reubenharry dbab9a3
UPDATE TESTS
reubenharry a7ffdb8
UPDATE TESTS
reubenharry 098f5ad
UPDATE TESTS
reubenharry 5bd2a3f
ADD DOCSTRING
reubenharry 4fc1453
ADD TEST
reubenharry 3678428
Merge branch 'inference_algorithm' into preconditioned_mclmc
reubenharry 203f1fd
STREAMING AVERAGE
reubenharry fc347d6
ADD TEST
reubenharry 49410f9
REFACTOR RUN_INFERENCE_ALGORITHM
reubenharry ffdca93
UPDATE DOCSTRING
reubenharry b7b7084
Precommit
reubenharry 9d2601d
RESOLVE MERGE CONFLICTS
reubenharry 97cfc9e
CLEAN TESTS
reubenharry 45429b8
CLEAN TESTS
reubenharry dd9fb1c
Merge branch 'preconditioned_mclmc' of https://github.com/reubenharry…
reubenharry a27dba9
GITIGNORE
reubenharry 7a6e42b
PRECOMMIT CLEAN UP
reubenharry 2d3c3fc
FIX SPELLING, ADD OMELYAN, EXPORT COEFFICIENTS
reubenharry dad0060
TEMPORARILY ADD BENCHMARKS
reubenharry 6bd5ab1
ADD INITIAL_POSITION
reubenharry 5615261
FIX TEST
reubenharry d66a561
Merge branch 'main' into inference_algorithm
reubenharry 290addc
Merge branch 'main' into inference_algorithm
reubenharry 35d4880
Merge branch 'inference_algorithm' into new_integrator
reubenharry 356cd3b
CLEAN UP
reubenharry 67c0002
Merge branch 'inference_algorithm' into preconditioned_mclmc
reubenharry 63a8042
REMOVE BENCHMARKS
reubenharry 51fee69
ADD TEST
reubenharry 29994d7
REMOVE BENCHMARKS
reubenharry e4be0ae
Merge branch 'preconditioned_mclmc' into new_integrator
reubenharry 64948e5
BUG FIX
reubenharry c3d44f3
CHANGE PRECISION
reubenharry 94d43bd
CHANGE PRECISION
reubenharry 17b7454
Merge branch 'preconditioned_mclmc' into new_integrator
reubenharry 636ef43
ADD OMELYAN TEST
reubenharry 178b452
RENAME O
reubenharry 9c1c816
Merge branch 'inference_algorithm' of github.com:reubenharry/blackjax…
reubenharry db90cdc
Merge branch 'inference_algorithm' into preconditioned_mclmc
reubenharry a26d4a0
UPDATE STREAMING AVG
reubenharry 0ff1d24
Merge branch 'preconditioned_mclmc' into new_integrator
reubenharry 4e2b7c0
MERGE
reubenharry 6bacb6c
UPDATE PR
reubenharry 654dacc
Merge branch 'preconditioned_mclmc' into new_integrator
reubenharry 9c2fea7
RENAME STD_MAT
reubenharry c249a12
Merge branch 'preconditioned_mclmc' into new_integrator
reubenharry 06dd04d
MERGE MAIN
reubenharry 88b06cb
MERGE MAIN
reubenharry abe707c
REMOVE COEFFICIENT EXPORTS
reubenharry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
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.
Remove? Are you planning to use it outside of integrators?
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.
Yes, I have found it convenient to use them outside integrators. For example, if I want to run scripts that try different integrators, and I want access to their number of gradient calls. I suppose the other option would be to have a dictionary like
{"velocity_verlet": {"num_grads": ..., "name": ..., "order": ...} }
. Is that what you'd recommend?As another example, I often want to do benchmarks against different integrators, but want to just iterate over
X_coefficients
, and then usegenerate_isokinetic_integrator
for MCLMC andgenerate_euclidean_integrator
for HMCThere 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.
Also, it seems like code duplication to have
isokinetic_velocity_verlet
,velocity_verlet
, etc, when we could just use the coefficients withgenerate_isokinetic_integrator
andgenerate_euclidean_integrator
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.
The design choice is motivated by how we are envisioning the usage of integrators in the library. Currently the design is to have integrator being a static set.
You should iterate through the integrator objects instead
Yes given that these are static, you should put them in your script as static parameters. I dont yet see those are useful in the library outside of benchmarking.
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.
OK, that makes sense. I think my one point of disagreement is that if I don't expose the coefficients, nothing in the code "knows" that
velocity_verlet
andisokinetic_velocity_verlet
are related. So I will have to have a dictionary of{"euclidean": velocity_verlet, "isokinetic": isokinetic_velocity_verlet, ...}
when I want to compare each integrator on hmc vs mclmc, which I'm currently doing. This is a little painful, but not the end of the worldThere 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.
Updated, as per request