search

choderalab / pinot

Probabilistic Inference for NOvel Therapeutics
MIT License
15 stars 2 forks source link

AdLaLa only one group? #35

Open yuanqing-wang opened 4 years ago

yuanqing-wang commented 4 years ago

@maxentile

seems that in this version of AdLaLa we have to have two groups? would this be a little bit more confusing then the previous dictionary key setup?

maxentile commented 4 years ago

I'm not sure I understand the question: is there more context?

yuanqing-wang commented 4 years ago

sorry by group I meant the param_group attribute of the optimizer. in this implementation we assumed there're two. but this is not necessarily the case.

maxentile commented 4 years ago

would this be a little bit more confusing then the previous dictionary key setup?

Feel free to revert or modify anything there -- I had started refactoring in this way when you requested a review ( https://github.com/choderalab/pinot/issues/6 and https://github.com/choderalab/pinot/pull/7 ), because I needed to see more clearly the substeps and the order in which they were applied. I think this was helpful in highlighting an ambiguity we later resolved with the authors, but it may have introduced unintended conflicts relative to behavior you were relying on.

in this implementation we assumed there're two. but this is not necessarily the case.

Agreed. Another goal in mind when breaking the substep definitions out of the step body was to make it easier to generalize later to arbitrary orderings of langevin or adaptive-langevin steps applied to many param groups. Note that the substep functions each accept an integer, indexing the param group to update, and do not themselves assume a fixed number of param groups.

The body of step however has a straight line of calls that update only group indices 0 and 1 (note that this assumes two groups, not one group), in the fixed order proposed in the paper. I imagined you might want to inherit from this class and override the step definition while retaining the substep definitions. For example, you may want to accept a user-specified splitting, rather than a specific hard-coded sequence (just as we did for force-group-splitting integrators in openmmtools). You could also override step to do the updates in an order that's determined by "partition" attributes of each param group, recovering the logic of your original implementation.

yuanqing-wang commented 4 years ago

I really like the way you implement sub-steps first and then glue them together in a clear schedule. I'll simply replace 0 and 1 by a keyword.