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

Replace pair_id in neighbors list samples with the actual cell shift #221

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

Luthaf
Copy link
Member

@Luthaf Luthaf commented Aug 21, 2023

This is more informative, and not that much work overall.

@DivyaSuman14 and @HannaTuerk needed something like this for the hamiltonian learning. You can even compute only one of the translated cells at the time by using sample selection!


@felixmusil, this should solve #131 (comment), with all the data you want being in the samples:

calculator = rascaline.NeighborList(cutoff=3.4, full_neighbor_list=True)
descriptor = calculator.compute(frames)
descriptor.block(0).samples
# Labels(
#     structure  first_atom  second_atom  cell_shift_a  cell_shift_b  cell_shift_c
#         0          0           37            0             0             0
#         0          37           0            0             0             0
#         0          1            2            0             0             0
# ...
#         0          51          51            0             0             0
#         0          52          52            0             0             0
#         0          53          53            0             0             0
# )

# in ASE terms:
i = descriptor.block(0).samples["first_atom"]
j = descriptor.block(0).samples["second_atom"]
S = descriptor.block(0).samples.view(["cell_shift_a", "cell_shift_b", "cell_shift_c"]).values
D = descriptor.block(0).values

Ping @rosecers I think you are also using the neighbors list calculator so these changes might impact you (only if you are explicitly manipulating the old pair_id sample)


📚 Documentation preview 📚: https://rascaline--221.org.readthedocs.build/en/221/

@felixmusil
Copy link

Looks perfect, great work !

Copy link
Collaborator

@rosecers rosecers left a comment

Choose a reason for hiding this comment

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

Nice! @arthur-lin1027 can you check if this makes any problems with anisoap?

@Luthaf Luthaf force-pushed the better-neighbors-list-samples branch from 9adea82 to 505ddee Compare August 22, 2023 14:57
@github-actions
Copy link

github-actions bot commented Aug 22, 2023

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

@Luthaf Luthaf requested a review from PicoCentauri August 23, 2023 13:57
@arthur-lin1027
Copy link

arthur-lin1027 commented Aug 23, 2023

Nice! @arthur-lin1027 can you check if this makes any problems with anisoap?

@rosecers I ran the code on dev424 (my locally installed version) and dev450 (this current version) and I got the same results! Makes sense since we aren't manipulating the pair_id sample, as Guillaume mentioned above.

@Luthaf Luthaf force-pushed the better-neighbors-list-samples branch from 505ddee to 7c5f6ce Compare August 31, 2023 15:44
@Luthaf Luthaf requested a review from PicoCentauri August 31, 2023 15:44
@Luthaf Luthaf force-pushed the better-neighbors-list-samples branch from 7c5f6ce to a9f7bda Compare September 6, 2023 15:32
Copy link
Contributor

@PicoCentauri PicoCentauri 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 incoperating the comments. LGTM!

@Luthaf Luthaf merged commit 6970341 into master Sep 19, 2023
24 checks passed
@Luthaf Luthaf deleted the better-neighbors-list-samples branch September 19, 2023 11:44
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.

6 participants