search

coarse-graining / cgnet

learning coarse-grained force fields
BSD 3-Clause "New" or "Revised" License
57 stars 26 forks source link

Simplifying prior index callbacks and interfacing with custom features #60

Closed nec4 closed 5 years ago

nec4 commented 5 years ago

Development:

Work in progress This branch simplifies prior layer instantiation by directly passing feature indices to __init__() methods. It also adds tests that certify integrability with #58, where in custom features and feature order may be specified fro feature layers.

brookehus commented 5 years ago

Let's make sure to keep actively discussing this as I work on #58. Especially some weird stuff is gonna happen with bonds. Also I like the phrase "certify integrability".

brookehus commented 5 years ago

I've been thinking of the "needs review" label as like, ready to review. Do you want me to review this or wait?

nec4 commented 5 years ago

Good points. I think I jumped the gun on the needs review label. I think its best to wait until you are ready to review after you are satisfied with #58. Shall we save the label for those PRs that need review immediately?

brookehus commented 5 years ago

Yes I think it should be added once it should/is ready to be reviewed but hasn't yet been

brookehus commented 5 years ago

Does this need another pull from betas?

nec4 commented 5 years ago

Regarding the pull: I rebased to betas and made a few changes to GeometryStatistics.return_indices() and GeometryStatistics.get_prior_statistics(). Basically, the users can now specify an arbitrary list of bead tuples and get the statistics back for just those. I struggled for a while to decide how to handle dihedrals. In the end I chose to adopt the 'sin'/'cos' label when the user asks for a feature index/stats info. For example, asking for the index/stat info for (0,1,2,3) will raise a ValueError, but (0,1,2,3,'sin') or (0,1,2,3,'cos') will work. This may be subjectively ugly, but I think it eliminates some inherent ambiguity with the dihedrals, and GeometryStatistics.get_prior_statistics() already used this convention in its return value via GeometryStatistics_get_key. Need to fix some tests next, but I think we're almost there!

brookehus commented 5 years ago

I want to review this but the diff still looks weird (shows that betas still uses PBF and _inds names, for example) Not sure why. I copied the branch to prior-fix-beh and pulled from betas to make a cleaner diff/new PR

brookehus commented 5 years ago

61 still has all your commits so can you check it out and see if we can move to that? Do you see the differences in the diffs? Anyway, I'll put my comments there for now.

brookehus commented 5 years ago

Added bug label because of weird diffs. Check out #61 and #62.