Skip to content

Conversation

Linford24
Copy link

No description provided.

Copy link
Author

@Linford24 Linford24 left a comment

Choose a reason for hiding this comment

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

A check for same atom binding to itself was added to both the BondList constructor and add_bond method.

@Linford24 Linford24 marked this pull request as ready for review September 12, 2025 14:59
@Linford24 Linford24 closed this Sep 12, 2025
@Linford24 Linford24 reopened this Sep 12, 2025
@Linford24 Linford24 changed the title Contribution A check for same atom binding to itself was added to both the BondList constructor and add_bond method Sep 12, 2025
@padix-key padix-key changed the title A check for same atom binding to itself was added to both the BondList constructor and add_bond method Disallow redundant bonds in BondList Sep 12, 2025
@padix-key
Copy link
Member

Addresses first part of #812

Copy link
Member

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. However, I see a problem with the performance: Iterating over bonds directly happens in pure Python, which can be quite time consuming. Hence I'd rather suggest to use a memoryviewinstead which cython compiles into C code.

Are you familiar with these Cython details? Otherwise I also can make the amendments here.

@Linford24
Copy link
Author

Yes please. This PR only addresses the first part. The second part regarding the chem_comp bond will be in a separate PR as you suggested and I will be push soon

@Linford24
Copy link
Author

I am not familiar with the Cython details but I would be glad if you could brief me on them so I implement the suggestion myself and get to learn it as well

@Linford24
Copy link
Author

Meanwhile, I am reading about the change you suggested. Would get back with the revised amendments

…ad of directly iterating over bonds in pure Python.
Copy link
Author

@Linford24 Linford24 left a comment

Choose a reason for hiding this comment

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

Added iteration over bonds with memoryview instead of pure Python

@padix-key
Copy link
Member

Sorry for the late response, I was sick for a few days. I think these two tutorial pages give a sufficient overview of Cython's memory views:

https://cython.readthedocs.io/en/latest/src/userguide/numpy_tutorial.html
https://cython.readthedocs.io/en/stable/src/userguide/memoryviews.html

If you look at bonds.pyx you will often encounter lines, like these:

cdef uint32[:,:] all_bonds_v = self._bonds

It tells Cython to convert self._bonds into a typed memoryview of uint32 values, which can then be efficiently accessed in C-code, without expensive calls to the Python API. You can access the dimensions of the memoryview using .shape and use integer indices to access specific elements. Note that fancy indexing is not possible for these constructs

@Linford24
Copy link
Author

I'm much glad you're fit again. I'm going through the tutorials

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