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

Unexpected behavior of CrystalNNFingerprint #347

Open
utf opened this issue Dec 17, 2018 · 1 comment
Open

Unexpected behavior of CrystalNNFingerprint #347

utf opened this issue Dec 17, 2018 · 1 comment

Comments

@utf
Copy link
Member

utf commented Dec 17, 2018

The output of CrystalNNFingerprint does not match up with the output of the CrystalNN class.

For example the following code snippet:

import numpy as np
from matminer.featurizers.site import CrystalNNFingerprint
from pymatgen.ext.matproj import MPRester
from pymatgen.analysis.local_env import CrystalNN

s = MPRester().get_structure_by_material_id("mp-707852")
s.add_oxidation_state_by_guess()
bs = CrystalNN().get_bonded_structure(s)

site_of_interest = 51

cnnf = CrystalNNFingerprint.from_preset("ops")
features = cnnf.featurize(s, 51)

sort_indices = np.argsort(features)
top_id = sort_indices[-1]
second_top_id = sort_indices[-3]

print("coordination of site {}: {}".format(
    site_of_interest, bs.get_coordination_of_site(site_of_interest)))
print("top feature: {} with OP: {}".format(
    cnnf.feature_labels()[top_id], features[top_id]))
print("second top feature: {} with OP: {}".format(
    cnnf.feature_labels()[second_top_id], features[second_top_id]))

Produces the output:

coordination of site 51: 2
top feature: wt CN_1 with OP: 0.6286992249320102
second top feature: wt CN_3 with OP: 0.3295052806667878

The coordination of site 51 obtained by the CrystalNN class does not match up with the coordination indicated by CrystalNNFingerprint. Even the second highest order parameter is for the wrong coordination number.

@ardunn
Copy link
Contributor

ardunn commented Feb 12, 2019

@utf any update on why this might be? Fixing this issue might improve automatminer as a secondary effect

@ardunn ardunn mentioned this issue Jun 9, 2020
15 tasks
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 a pull request may close this issue.

2 participants