-
Notifications
You must be signed in to change notification settings - Fork 18
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
add bonds to AbstractSystem #90
base: master
Are you sure you want to change the base?
Conversation
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.
Left some comments, focusing on the interface itself for now. Once we have consensus on that, we should also make sure to add some tests for the BondedSystem
type.
Returns a Vector{Tuple{Integer, Integer, BondOrder}} where each entry stores the unique indices | ||
i and j of the atoms participating in the bond and the order of the bond (e.g. [(13,21,5)]) | ||
""" | ||
function bonds end |
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.
Can you add a default implementation that just returns an empty tuple? e.g. instead of this line, something like:
bonds(sys::AbstractSystem) = [()]
(I'm not sure if there's a type stability issue here with that return type not having the type parameters of the Tuple, though)
bonds(::AbstractSystem) | ||
|
||
Returns a Vector{Tuple{Integer, Integer, BondOrder}} where each entry stores the unique indices | ||
i and j of the atoms participating in the bond and the order of the bond (e.g. [(13,21,5)]) |
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 think the example at the end of this line is actually in line with what you want, since the third element should be a BondOrder
enum...
""" | ||
bonds(::AbstractSystem) | ||
|
||
Returns a Vector{Tuple{Integer, Integer, BondOrder}} where each entry stores the unique indices |
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.
Strictly, Integer
should probably be typeof(firstindex(sys))
(in practice, I think all implementations do use integer indices right now, but this is more general)
single | ||
double | ||
triple |
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.
should probably do this as
@enum BondOrder begin
single = 1
double = 2
triple = 3
end
because it seems that if you don't do that, it actually assigns them numbers in a zero-indexed fashion (super weird), so the following can happen, which would be very confusing:
julia> BondOrder(2)
triple::BondOrder = 2
Separately, there might be a longer-term discussion about whether we should break out bonding types to be part of the interface since I could imagine people wanting more flexibility here, but my opinion is we start with this and file an issue for future conversations about this.
My suggestion would be to not have the bond order as a required part of the interface. Without this it becomes a more general interface for storing atom pairs. Instead we could have properties of bonds like we have properties of atoms, and some way to ask for the properties when calling |
Added
bonds
function toAbstractSystem
and an example of aBondedSystem
. The example is basically just aFastSystem
. I have not added tests to this yet.This is based on discussion from #87.