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

Generalized coordination #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kareem-Abdelmaqsoud
Copy link

I just added the this function for calculating the generalized coordination numbers. The only thing I do is multiplying the connectivity matrix by the coordination numbers, getting a coordination matrix, and I then sum it up along one axis.
I also added a testing.ipynb to check the calculated CN and GCN values

  • so far the results of both CN and GCN are incorrect

@jkitchin
Copy link

What does "so far the results of both CN and GCN are incorrect" mean?

@kareem-Abdelmaqsoud
Copy link
Author

kareem-Abdelmaqsoud commented Mar 10, 2023

sorry, for being unclear. I thought only Brook is working on this. So, I explained what I meant on slack. But basically if you look at the testing notebook, you will see that the coordination numbers for Cu(100) is 4 which is incorrect. It should be 8 instead. And similarly the GCN values are incorrect because they shouldn't be this low.

@kareem-Abdelmaqsoud
Copy link
Author

Brook and I are looking into why they are incorrect. I believe it's related to how the surface atoms are being tagged. In my project, I don't use the same way the to tag the surface atoms as here. I just label the atoms with CN<12 as surface atoms.

@brookwander brookwander self-requested a review March 10, 2023 17:30
@@ -0,0 +1,95 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

please delete this file

surface_atoms = self.slab[
[idx for idx, tag in enumerate(self.slab.get_tags()) if tag == 1]
]
connectivity_matrix = self._get_connectivity_matrix(surface_atoms).toarray()
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we assess the connectivity matrix over self.slab this should fix the descrepancy you have noticed. But in this case we need to make some changes to line 235 too. I actually wanted to find the CN of surface atoms only though so I can make a duplicate function to do what you are interested in for CN

[idx for idx, tag in enumerate(self.slab.get_tags()) if tag == 1]
]
connectivity_matrix = self._get_connectivity_matrix(surface_atoms).toarray()
cns = [sum(row) for row in connectivity_matrix]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be cns = [sum(row) for idx, row in connectivity_matrix if self.slab[idx].tag == 1] or something to that effect

Returns:
(dict): the generalized coordination info. ex. `{"mean": 5.5, "min": 5, "max": 6}
"""
surface_atoms = self.slab[
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is no longer necessary

"""
connectivity_matrix = self._get_connectivity_matrix(self.slab).toarray()
cns = np.sum(connectivity_matrix, axis=1)
surface_idx = np.array([idx for idx, tag in enumerate(self.slab.get_tags()) if tag == 1])
cn_matrix = np.array(cns)*connectivity_matrix
gcn = np.sum(cn_matrix, axis=1)/12
gcn = np.sum(cn_matrix, axis=1)/np.max(cn_matrix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kareem-Abdelmaqsoud The way this was it wasnt extensible to bulks with bulk cn values that arent 12. I still think this doesnt handle the case where the bulk has multiple coordination numbers. i.e. some lattice positions have cn = 12 and others have cn =8 for the same bulk. How should gcn be calculated in that case?

… cn_matrix instead of the maximum of the matrix in general.
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.

3 participants