-
Notifications
You must be signed in to change notification settings - Fork 647
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
basic guesser features #3753
base: develop
Are you sure you want to change the base?
basic guesser features #3753
Conversation
Hello @aya9aladdin! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-09-12 19:40:16 UTC |
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.
Hi @aya9aladdin, this is a neat start! I only had time to skim but left a couple comments. The tests are failing because _GUESSERS can't be imported -- it's not in the top level __init__.py
yet.
package/MDAnalysis/core/universe.py
Outdated
@@ -1436,6 +1439,19 @@ def from_smiles(cls, smiles, sanitize=True, addHs=True, | |||
|
|||
return cls(mol, **kwargs) | |||
|
|||
def guess_topoloyAttribute(self, context, to_guess): |
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.
def guess_topoloyAttribute(self, context, to_guess): | |
def guess_TopologyAttr(self, context, to_guess): |
There's already a method called add_TopologyAttr
, and the class is called a TopologyAttr
. Going with the principal of least astonishment that advises a consistent API, could you please rename this method to something that users could easily guess (😛) themselves?
context = 'default' | ||
|
||
def __init__(self): | ||
self._guess = {'mass': self.guess_masses} |
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.
Hmm. Instead of this, would it be tidier to have a class-level _guess
dictionary like TopologyAttr.transplants
? I'm not sure it makes sense for instances of guesser classes to differ.
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.
I'll check this and see how I could integrate the transplant idea
package/MDAnalysis/guesser/base.py
Outdated
.format(self.context, a)) | ||
return True | ||
|
||
def guessTopologyAttribute(self, to_guess): |
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.
In the absence of type hinting, could you please change the name of the to_guess
variables to be clearer? here it's one string, but in is_guessed
it's a list of strings -- that is quite confusing. Also, could you please name your methods with conventional snake_case and use TopologyAttr instead of TopologyAttribute?
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.
Do you plan to have an overall method for guessing all the attributes at once?
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'm planning to have a method for guessing all the attributes at once, this was just a test for guessing one attribute
package/MDAnalysis/guesser/base.py
Outdated
values = self._guess[to_guess]() | ||
return values | ||
|
||
def setAtoms(self, atoms): |
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.
Does this method need to exist? Could __init__
just take atoms instead, like a Writer
?
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.
I don't remember why I did it this way tbh, so I'll remove it
@aya9aladdin I recommend that you push your changes often. Do not wait for the code to be finished. By pushing often, we get to see your progress, we get to comment on your latest version instead of a version that may already be obsolete, and we may catch issues earlier, reducing the amount of work you will have to redo. |
I was planning to push everything when I finish indeed. I don't know if I have to make a pull request at this unmatured level or not? |
You already have a pull request, no need to open another one. Instead, update this pull request by pushing your new commits. |
I have made some updates: 2- some attributes depend on other attributes to be guessed. if those parent attributes also need to be guessed, this can raise unnecessary errors if we attempted to guess the child attribute before the parent one. So, I added a rank dictionary to the guesser class which will rank each attribute based on its dependency on parent attributes to be guessed. For example , next update will have: |
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.
I had a partial look through, I hope the minor comments help.
I'm just more concerned with the logic than the documentation at the moment that's why it's not very accurate |
remove guessing types and masses from parsers
…lysis into guesser-basics
I think the MR is ok now..... you can review and give me feedback @lilyminium @IAlibay ...... sorry for the late reply, it was a tough month for me :) |
Thanks @aya9aladdin and I'm sorry to hear about the difficult month :( I'll get a review in as soon as possible - there's a few things I need to check with @lilyminium first. |
Just a heads up @aya9aladdin we haven't forgotten about this, it's just been a busy time all around -- but @IAlibay and I are meeting soon! |
Thank you for the reminder! Let me know if u need any source of help or discussion |
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.
Couple of things, may follow-up with more after tomorrow's call.
package/MDAnalysis/core/universe.py
Outdated
---------- | ||
context: str or :mod:`Guesser<MDAanalysis.guesser>` class | ||
For calling a matching guesser class for this specific context | ||
to_guess: list[str] (optional, default ``['types', 'masses']``) |
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.
Two things here:
- Optionals should really be
None
rather than()
, it's a nit-pick but only in the sense that Optional[List] means Union[None, List] in Python - From the signature, the default is
()
not['types', 'masses']
.
@@ -303,8 +306,7 @@ def parse(self, **kwargs): | |||
if atomtypes: | |||
attrs.append(Atomtypes(np.array(atomtypes, dtype=object))) | |||
else: | |||
atomtypes = guessers.guess_types(names) | |||
attrs.append(Atomtypes(atomtypes, guessed=True)) | |||
atomtypes = elements |
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.
This needs to be upper case, since atomtypes are always upper case when guessed.
@aya9aladdin apologies for taking so long here again. @lilyminium and I finally managed to discuss this and came up with a plan forward here. As far as we think, if you can resolve the remaining issues / comments that have been raised, then we should be good to merge this. Please do let us know what you think! |
happy to hear that! will work on the recently raised issues/reviews at the weekend |
Hi @aya9aladdin, I hope you're doing well! I just wanted to check in and see how things are going with the PR. I know the cycle of review has gone on for quite a while, so if there's anything we can do to help, please don’t hesitate to let us know. We'd be more than happy to help out, e.g by opening additional PRs into this one. Thanks so much for all your efforts on this! |
Hi @lilyminium sorry for the late response! it was quite messy for me last week. Will check it today or tomorrow maximum and push needed updates |
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
- updated guess_TopologyAttrs docs - fixed some tests - capitalize atomtypes from elements in RDKitParser
@lilyminium @IAlibay I pushed last updates, let me know if any other updates is needed |
Thanks @aya9aladdin!! Will aim to get my review in by this weekend. |
I won't be able to re-review for a little while unfortunately, probably end of the week or next weekend. |
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.
Apologies for the delay, I have two remaining questions, but happy to approve.
@lilyminium would it be ok if you took care of shepherding the remaining things?
""" | ||
def __init__(self, topology=None, *coordinates, all_coordinates=False, | ||
format=None, topology_format=None, transformations=None, | ||
guess_bonds=False, vdwradii=None, fudge_factor=0.55, | ||
lower_bound=0.1, in_memory=False, | ||
lower_bound=0.1, in_memory=False, context='default', | ||
to_guess=('types', 'masses'), force_guess=(), |
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.
to_guess=('types', 'masses'), force_guess=(), | |
to_guess=('types', 'masses'), force_guess=None, |
Could you confirm that keeping ()
instead of None
was intentional?
See: #3753
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.
@lilyminium feel free to turn this into an issue, we can fix this in post.
@@ -303,8 +306,7 @@ def parse(self, **kwargs): | |||
if atomtypes: | |||
attrs.append(Atomtypes(np.array(atomtypes, dtype=object))) | |||
else: | |||
atomtypes = guessers.guess_types(names) | |||
attrs.append(Atomtypes(atomtypes, guessed=True)) | |||
atomtypes = np.char.upper(elements) |
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.
@aya9aladdin @lilyminium - this is assigned but nothing else is done with it afterwards it seems? I feel like I'm missing something, could you please just double check?
Fixes #
Changes made in this Pull Request:
PR Checklist