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

Code refactoring needed (replace next() usage) #53

Open
dominiquesydow opened this issue Jul 31, 2022 · 2 comments
Open

Code refactoring needed (replace next() usage) #53

dominiquesydow opened this issue Jul 31, 2022 · 2 comments
Assignees

Comments

@dominiquesydow
Copy link
Collaborator

dominiquesydow commented Jul 31, 2022

I think we are misusing next() in this context (it works but it is not the proper way to do this and whenever errors occur the traceback is a bit difficult):

KinFragLib/kinfraglib/utils.py

Lines 1297 to 1302 in 99e94c2

dummy_1 = next(
atom for atom in combo.GetAtoms() if atom.GetProp("fragment_atom_id") == bond[0]
)
dummy_2 = next(
atom for atom in combo.GetAtoms() if atom.GetProp("fragment_atom_id") == bond[1]
)

What are we doing here?
We are selecting an atom from a molecule based on a fragment atom id (stored in bond). This selection should result in exactly one atom. Then, we are using next() to extract this one atom from the list comprehension. This is problematic in two ways:
(1) What if the selection returns no atom? Throws StopIteration I think.
(2) What if the selection returns multiple atoms? Should never happen but we should add a check.

What do I suggest?
Remove next(). Store selection in a list and go through all cases with if-elif-else (0, 1, >1 atoms), raise proper errors for 0 and >1 atoms.

@dominiquesydow dominiquesydow self-assigned this Jul 31, 2022
@dominiquesydow
Copy link
Collaborator Author

For reference, this issue was prompted by #48.

@MachineGUN001
Copy link

I think we are misusing next() in this context (it works but it is not the proper way to do this and whenever errors occur the traceback is a bit difficult):

KinFragLib/kinfraglib/utils.py

Lines 1297 to 1302 in 99e94c2

dummy_1 = next(
atom for atom in combo.GetAtoms() if atom.GetProp("fragment_atom_id") == bond[0]
)
dummy_2 = next(
atom for atom in combo.GetAtoms() if atom.GetProp("fragment_atom_id") == bond[1]
)

What are we doing here? We are selecting an atom from a molecule based on a fragment atom id (stored in bond). This selection should result in exactly one atom. Then, we are using next() to extract this one atom from the list comprehension. This is problematic in two ways: (1) What if the selection returns no atom? Throws StopIteration I think. (2) What if the selection returns multiple atoms? Should never happen but we should add a check.

What do I suggest? Remove next(). Store selection in a list and go through all cases with if-elif-else (0, 1, >1 atoms), raise proper errors for 0 and >1 atoms.

when I try to run
4_3_combinatorial_library_comparison_klifs notebook
the same error occured.
Snipaste_2022-10-25_15-22-23

The strange thing is that I just simply run the original code without making any changes. What should I do?
many thanks,

@kabu00002 kabu00002 mentioned this issue Nov 15, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants