-
Notifications
You must be signed in to change notification settings - Fork 663
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
Implement fancy guessers (e.g., force-field aware) #598
Comments
So as a signature for this, I was thinking.. from MDAnalysis.core.guessers import guess_masses
masses = guess_masses(u) # should this add itself to Universe here?
u.topology.add_attribute(guess_masses(u)) # or like this so it's explicit For now this can follow the old system, but we should leave room for specifying a direction to guess in
|
I'm not sure I follow some of the details: Or do you mean to have the masses And finally, does |
Sorry, I think I got it wrong. We need to add it at Universe level (so it gets propagated into Groups) So the Guesser here is what creates the So.. guess_masses(u) # creates the TopologyAttribute
u.add_TopologyAttribute(guess_masses(u)) # creates and adds to Universe I'm using |
I still don't fully understand why this is a |
For creating the The attribute needs to be added to the Universe so that |
@mnmelo the |
I had a chat with @mnmelo about guessers a fews days ago. I would implement each guesser (mass, type, bonds...) as a method of a guesser class. The current implementation would go in some For the end-users, it allows to specify what kind of system he is about to work with and get the guess done accordingly. API-wise, I see something as follow: # By default, guess with the default guessers as we currently do
u = Universe(top, traj)
# We can explicitly ask for guesses, assuming the default guessers
u = Universe(top, traj, guess=True)
# We can explicitly ask for the default guessers
u = Universe(top, traj, guess=DefaultGuessers)
# We can make guesses for a Martini system
u = Universe(top, traj, guess=MartiniGuessers)
# We can deactivate guessing
u = Universe(top, traj, guess=False)
u = Universe(top, traj, guess=None)
# Would we provide several sets of guessers, they would be easily
# discoverable as the following line does work well with autocompletion:
from MDAnalysis.topology.guessers import MartiniGuessers For the developper, it allows to easily build on top of an existing collection of guesser by simply inherit from it. It also allows to have custom guessers in a different module. This last point is especially convenient as it allows to develop and maintain a collection of guessers separately from MDAnalysis. People developing force fields or parametrizing new molecules could keep their guessers in sync with their parameters regardless of MDAnalysis updates. In practice, it means for me that I can switch MDAnalysis branch, and still have the guessers for my force field developments. One could want to guess bonds but not masses. I most likely overlook some critical use cases. But this is what I came with after discussing with @mnmelo . |
This ties in with #942. By default, we don't want all guessing to be active, right? Bonds/angles aren't guessed by default right now. This means that the default is roughly
Could we also allow the use of Guessers instances along with Guessers classes?
And what if I already have a |
I see nothing against this.
It could be something like |
We would probably want shortcuts, then, right? class Universe(object):
...
def guess_masses(self, guessers=MDAnalysis.guessers.DefaultGuessers):
return self.guess(guessers, to_guess=['masses']) |
We could have shortcuts of course, but we still need the generic method. Indeed, with the new topology scheme we can have any arbitrary topology attribute attached to a system. One can therefore have a guesser that guesses the propensity of atoms toward chocolate cakes. We do not want a |
This all sounds good. I'd put them in We could do another register trick with the Guesser class (like we do with Readers) so that the name of the Guesser gets added to # in guess_martini.py
class MartiniGuesser(BaseGuesser):
basis_name='martini'
# somewhere else
u.guess(basis='martini', to_guess=['masses', 'sigma', 'epsilon']) But maybe this is isn't necessary..... One little thought, it should be made easy to supplement an existing guesser, ie if I've added an atomtype, but maybe this is easily done if the variables are class attributes g = mda.guessers.MartiniGuesser
g.masses['me'] = 100.0
g.charge['me'] = -1.5
u.guess(g, to_guess=['mass', 'charge']) |
We could do the register trick assuming we can still pass a class directly. I am almost sure I will end up with 5 different versions of a martini guesser in some messy notebook. I agree that the guessers should be as easy as possible to extend. I did not think about these details yet, but your exemple make sense. |
I'm writing up an implementation of this API. Stay tuned... |
@mnmelo are you still free to do this or is this open to all? |
I left this open pending the decisions in #942. If you have availability I'd say go ahead because mine will certainly be flaky. |
From the comments in 1524f75 we may have to think about format-aware guessing, in which guessing might depend on the topology being used. I'm not sure I like the loss of abstraction in this. |
@richardjgowers wasn't there some work about this already in the topology refactor branch? |
All I did was reimplement the existing guessers in topology/guessers. I
didn't do any of the fancy and cool things discussed here
…On Sun, 27 Nov 2016, 11:55 a.m. Max Linke, ***@***.***> wrote:
@richardjgowers <https://github.com/richardjgowers> wasn't there some
work about this already in the topology refactor branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#598 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AI0jB5Rvu7Y27NJflMyCJZfMXTg0aR0Rks5rCW_CgaJpZM4G7m9M>
.
|
@richardjgowers aren't guessers implemented now? |
The guessers we've always had are still there, I think this issue refers to having cool ones like outlined here. |
Can this issue be closed with PR #3704 merged? It would be cleaner to raise issues for individual contexts/guessers that experts could start working on. (ping @lilyminium @IAlibay ) |
So with #363 we removed guessing of various things in Parsers (ie whilst reading the files) and deferred this guessing to be after Universe init.
The guessing now needs to be attached to Universe. These need to create the appropriate Attribute and add it to Universe (
u.add_TopologyAttr
). So you should be able to call something likeu.guess_masses()
.Guessers required for:
PR onto the issue-363 branch
The text was updated successfully, but these errors were encountered: