Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Structure graphs #63
Structure graphs #63
Changes from 33 commits
6a66c87
eb45aa7
ad74c3c
91e1548
b051f1b
0278753
35dd472
6b2cec3
6359234
04a4096
8dff9a2
cc9d6e7
074c871
7d45f13
7885eae
613da7b
9cc12cf
7dfa7ba
7f60606
33fda06
7b114d1
94aa13f
e69e0b7
26d55ae
544e8a9
7f7258a
cb915d2
eed80ae
e6c6a1f
472c405
4f90dc4
30bdf25
8dcdb44
3fa39f5
017c50e
d3c193f
c0f3c56
deab80a
283dfa0
466b9d8
e07aa20
64b108a
442288e
d3e1828
8045c75
af44445
49d3912
e59f506
6f2d8c5
27142fc
091aa42
a1383f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Isn't the charge information already in LobsterNeighbors or the Analysis object or anything else?
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.
Oh yes, I just looked up into get_structure_with_charges method, I infact do not need to reinitialize the charge object just for that method. Thanks for the suggestion. Will implement the changes soon
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.
Ops, we cannot, Lobsterpy only has Mulliken charges, Loewdin Charges are not part of Analysis object
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.
I see! Thanks for checking!
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.
Isn't LobsterNeighbors part of the Analysis object? Can't you reuse this? I think we are currently running the same code multiple times which could be really slow.
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.
Indeed
LobsterNeighbors
is part of Analysis object which can be reused, but as it is not initialized with ICOOP and ICOBI info when we create the analyze object. So the structure graph will not have ICOBI and ICOOP as edge properties.I think to keep it simple we can simply not have them in single structure graph object?
It was implemented like this as ICOOP and ICOBI extension was not implemented before. Now we can simply have different sg objects for based on different analysis objects.
Let me know what you think of this? I will then make changes there
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.
Can't you add a kwarg or something to Analysis to pass the ICOBI, ICOOP to LobsterNeighbors stuff?
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.
I can, but I think it can be quite confusing if we are already planning to extend the analysis to COBIs and COOPs.
If the user does analysis for COBICAR, then sg will not have COHP info added with our current implementation in pymatgen. I will need to first update LobsterNeighbors if we want to go this way.
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.
Ideally, we can determine bonds based on ICOHP, ICOBI and add ICOHP, ICOBI values to each structure graph. I guess we will keep the current implementation for now and then think about improvements later.
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.
Thanks for checking!
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.
Yes, I agree 😄
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.
You are welcome
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.
Mayybe add a comment to describe what you are doing in each step?
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.
Added some additional comments as requested. Hope these do not cause linit errors , I just added via GUI on git
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.
I think we need to add precommit to the workflows at some point.
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.
Yes
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.
Will do add it next time