Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Neighbour analysis for the entire trajectory #251
Neighbour analysis for the entire trajectory #251
Changes from 15 commits
f7648be
f8f2a95
bb8b781
84c82ac
2308729
b6456c1
aa5f24f
02611d1
6bc829b
4e1cb35
cf219f8
3341f0d
cc0ec61
80cb83e
9614ef8
7462d71
f349cf7
13c5978
2e12c1e
5f407a2
3d9792e
e5cfc43
b278379
d9cc947
9acaa44
7c00690
02413e0
ecad9c0
4aa7856
7f23b96
ed5e4f5
c6b9fed
ae725e5
1b74ecc
62e39a4
6d51b09
853b376
2d507db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think it'd be extremely useful for this to take any object that implements
HasStructure
instead of passing all of these things separately. Callingget_structure
every step might be a bit slower, but since you're doing essentially whatAtomisticGenericJob.get_structure
does anyway, it shouldn't be that much.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.
If it turns out much slower, then I'd say we add a method roughly like this
to
NeighborTraj
to achieve the same generality, but offer a small escape hatch for cases where we knowget_structure
is too slow.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 think this is a great way to generalize this for any derivative of
HasStructure
. However, I don't think building the positions and cells from the individual structures is not that efficient.So how about something like this:
and then
which gets called instead of
_get_neighbors
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 think we should simply give it a try to use
get_structure()
everywhere and see if it really is slower. CheckingTrajectory._get_structure
and your code in_get_neighbors()
looks essentially the same to me, so I don't expect a big hit.If you don't want to do it, I can give it a go tonight.
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.
Also
__init__
needs to accept aninit
argument as the first argument and pass it tosuper().__init__()
, otherwise recursive loading from HDF will not work.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.
Sure I'll merge your PR into this one and see what happens!
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.
Not sure what you mean by this. Could you clarify?
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.
When you derive from
DataContainer
the new__init__
method must be compatible to the original one. One of the waysDataContainer
is instantiated is by passing a collection to initialize it, like soBecause this is used by the
DataContainer
internally when loading from hdf, subclasses also need to offer this. There's a little bit more explanation here in the attention box.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.
For
__init__
to be compatible a change like this would be necessary. Wheninit
is given the other parameters are not set, because presumably they are set in theinit
read from HDF5. If you want to be complete, you can check thatinit
really contains all the attributes that__init__
normally sets and set those manually that are not in `init.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.
Since the only purpose of this class is to call this method, can we just call it in
__init__
automatically (when it is not loaded from HDF)?