Skip to content
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

Fix atom labeling so they can be named in constraints #226

Closed
wants to merge 7 commits into from

Conversation

fabgonzal
Copy link

Description

Old behavior named ligand atoms by element. Added 1-indexed index to element symbol.

Motivation

Atoms require a unique label so they can be specified in constraints.

Atom labels were not unique which prevented their use for constraints
@jackdent jackdent requested a review from wukevin December 9, 2024 02:01
for atom in mol_with_hs.GetAtoms():
atom.SetProp("name", atom.GetSymbol())
for i, atom in enumerate(mol_with_hs.GetAtoms()):
atom.SetProp("name", atom.GetSymbol()+str(i+1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a great addition, but I think we can modify this slightly to better conform to prior naming conventions. In particular, instead of numbering every atom in strictly ascending order, let's instead number every atom with as the i-th element.

Something like:

element_counter = defaultdict(int)
for i, atom in enumerate(mol_with_hs.GetAtoms()):
    elem = atom.GetSymbol()
    element_counter[elem] += 1
    atom.SetProp("name", elem + str(element_counter[elem]))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I made a bit of a mess of it but I think it should pass now.

@wukevin
Copy link
Contributor

wukevin commented Dec 10, 2024

Due to persistent errors in this PR, I've implemented the logic here in a separate PR; we'll close this one out when that merges.

#236

@wukevin wukevin closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants