-
Notifications
You must be signed in to change notification settings - Fork 16
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
Slicing support #67
Slicing support #67
Conversation
Cool! I'd like to see others' thoughts and perhaps discuss a bit, but I'm certainly preliminarily in favor of merging this (or something like it) in! |
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.
Nice idea. Certainly in favour of having something like this.
Looks good, definitely in favour of some version of this. |
- Now only getindex for Integer need to be defined for concrete types and all other slicing automatically work. - Remove the specific implentations from concrete types, - Add tests for getindex with 2D arrays.
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.
LGTM, just found a few nits.
Now with the key-based property interface we can do
system[:, :property]
to get the corresponding:property
for all atoms. Wouldn't it also make sense to supportsystem[rng, :property]
for a subset,rng
of atom indices. Also, from the discussion in #29, I think returning a list of particle objects for slicing is more intuitive and useful. In this PR, I have implemented both of these, i.e., returning vector of particles objects on slicing AbstractSystems and the corresponding extension to the key-based property interface.For example, with this, we can do
system[atomic_symbol(system) .== :H]
to get all H atoms.I am just putting this here as a working version to show the usefulness of this feature. Feel free to close this in case we think slicing need not be supported.