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

Speeding up get_nn_info in local_env.py #3635

Merged
merged 5 commits into from Feb 20, 2024

Conversation

ftherrien
Copy link
Contributor

_get_image and _get_original_site do not need to recompute site and index when site is a PeriodicNeighbor which has them as attributes.

When there are several sites, this makes a significant difference on speed for get_nn_info

Alternative implementation would have been try/except and hasattr. try/except is much slower when most of the sites are not PeriodicNeighbor.

Summary

Major changes:

  • feature: image and site index are not recomputed when they are already an attribute of the site

`_get_image` and `_get_original_site` do not need to recompute site and index when `site` is a `PeriodicNeighbor` which has them as attributes.
@JaGeo
Copy link
Member

JaGeo commented Feb 19, 2024

Awesome. Thanks! I recently tried the minimum distance method for an amorphous structure 😅...
And it took forever.

@janosh
Copy link
Member

janosh commented Feb 20, 2024

@ftherrien thanks, much appreciated! the failing test here looks like maybe this doesn't handle translations correctly? shifting the I atom by a full $a$ lattice vector shouldn't make a difference.

TestCrystalNN.test_shifted_sites _______________________

self = <tests.analysis.test_local_env.TestCrystalNN testMethod=test_shifted_sites>

    def test_shifted_sites(self):
        cnn = CrystalNN()
    
        sites = [[0.0, 0.2, 0.2], [0, 0, 0]]
        struct = Structure([7, 0, 0, 0, 7, 0, 0, 0, 7], ["I"] * len(sites), sites)
        bonded_struct = cnn.get_bonded_structure(struct)
    
        sites_shifted = [[1.0, 0.2, 0.2], [0, 0, 0]]
        struct_shifted = Structure([7, 0, 0, 0, 7, 0, 0, 0, 7], ["I"] * len(sites_shifted), sites_shifted)
        bonded_struct_shifted = cnn.get_bonded_structure(struct_shifted)
    
>       assert len(bonded_struct.get_connected_sites(0)) == len(bonded_struct_shifted.get_connected_sites(0))
E       assert 1 == 2

ftherrien and others added 2 commits February 20, 2024 11:25
…_sphere

If the modulo is present the image returned by get_points_in_sphere does not point to the original site.
@janosh janosh added performance Some functionality is too slow or regressed analysis Concerning pymatgen.analysis nn Nearest neighbor analysis labels Feb 20, 2024
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks again @ftherrien!

@janosh janosh enabled auto-merge (squash) February 20, 2024 16:54
auto-merge was automatically disabled February 20, 2024 17:21

Head branch was pushed to by a user without write access

@ftherrien
Copy link
Contributor Author

@janosh the auto-merge was cancelled FYI

@JaGeo JaGeo merged commit 38b9b58 into materialsproject:master Feb 20, 2024
22 checks passed
@JaGeo
Copy link
Member

JaGeo commented Feb 20, 2024

I have merged now, @janosh , as you approved it already.

@ftherrien
Copy link
Contributor Author

Thanks y'all!

@ftherrien ftherrien deleted the nn_info_speedup branch February 20, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Concerning pymatgen.analysis nn Nearest neighbor analysis performance Some functionality is too slow or regressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants