Structure graphs #63

naik-aakash · 2022-08-24T13:24:56Z

Hi Janine,

This is initial draft for getting structure_graphs .
I did add environment information from lobsterpy to graph nodes

Pending

Add test cases
Add bonding-antibonding percentages for COHP

Also provide feedback on the way it is implemented.

JaGeo · 2022-08-24T13:34:12Z

Thanks. Will check it out. And, yes, pymatgen could be changed.

naik-aakash · 2022-08-24T15:29:33Z

Added the bonding-antibonding percentages as edge properties. The integral values obtained, however, don't match very well.

So I still need to recheck. If you spot any error when reviewing, I would be happy to rectify it.

JaGeo

Thank you very much, Aakash. This is already a great start as always :). I have some suggestions and we can of course talk about it in more detail.

JaGeo · 2022-08-25T06:23:17Z

lobsterpy/structuregraph/__init__.py

+
+    def __init__(
+        self,
+        path_to_calcdir: str,


I think there is a chance that people have the files in different folders. I think we should support it.

Yes, I thought it would be bit faster to use, and number of argument lists is bit large to type it out. But what you said also makes sense. So will revert it back to initial style.

I think there is a chance that people have the files in different folders. I think we should support it.

Yeah, have removed the hardcoding , so users can give path to files seperately.

JaGeo · 2022-08-25T06:24:16Z

lobsterpy/structuregraph/__init__.py

+    def __init__(
+        self,
+        path_to_calcdir: str,
+        add_additional_data_sg=True,


Maybe, we can already add an option to decide that COHP data is used for the determination of the bond strengths and bonds.

Thanks @naik-aakash . I think it looks qvery good. Could you maybe make the suggested changes? I think, I can then merge it.

Hi Janine, Thnaks! The change suggested pending is for option to decide that COHP data used for the determination of the bond strengths and bonds. I think, I have made other suggested changes already. Will try to get it done as quickly as possible.

lobsterpy/structuregraph/__init__.py

… all/cation-anion bond

naik-aakash · 2022-08-26T13:44:40Z

Added test case for graph module . Let me know if any further changes are needed. Tests failed because current pymatgen release does not have bond_label key in sg object

JaGeo · 2022-11-01T14:54:39Z

Thanks @naik-aakash . I think it looks qvery good. Could you maybe make the suggested changes? I think, I can then merge it.

lobsterpy/structuregraph/graph.py

JaGeo · 2022-11-01T14:52:14Z

lobsterpy/structuregraph/graph.py

+        )
+
+        cba = analyze.condensed_bonding_analysis
+


Mayybe add a comment to describe what you are doing in each step?

Added some additional comments as requested. Hope these do not cause linit errors , I just added via GUI on git

I think we need to add precommit to the workflows at some point.

Will do add it next time

Added comments

naik-aakash · 2023-04-17T15:26:57Z

Hi @JaGeo, let me know if anything more needs to be added or modified here. 😄

JaGeo · 2023-04-17T16:04:35Z

Hi @naik-aakash , how does the test coverage currently look like and how diverse were the structures you tested the code on so far?

naik-aakash · 2023-08-25T15:43:42Z

Hi @JaGeo, this PR could also be reviewed and merged

JaGeo

Hi @naik-aakash ,

thanks for writing this! Could you check if some of the objects aren't already part of Analysis? Charge or LobsterNeighbors? And, if there are ways to simplify the code and potentially speed it up.

docs/source/conf.py

lobsterpy/structuregraph/graph.py

JaGeo · 2023-09-13T06:08:31Z

lobsterpy/structuregraph/graph.py

+            )
+
+        # Adds Mulliken and Löwdin charges as site properties to structure object (node properties)
+        decorated_structure = Charge(self.path_to_charge).get_structure_with_charges(


Isn't the charge information already in LobsterNeighbors or the Analysis object or anything else?

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

Ops, we cannot, Lobsterpy only has Mulliken charges, Loewdin Charges are not part of Analysis object

I see! Thanks for checking!

JaGeo · 2023-09-13T06:09:58Z

lobsterpy/structuregraph/graph.py

+        )
+
+        # Initialize automating bonding analysis from Lobsterpy based on ICOHP
+        analyze = Analysis(


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.

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

Can't you add a kwarg or something to Analysis to pass the ICOBI, ICOOP to LobsterNeighbors stuff?

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.

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.

Thanks for checking!

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.

Yes, I agree 😄

Thanks for checking!

You are welcome

Co-authored-by: J. George <JaGeo@users.noreply.github.com>

naik-aakash · 2023-09-13T07:53:50Z

@JaGeo, I have made the suggested changes.

In the following days, I will try to make changes in pymatgen to switch to any icoxxlist as a base file for neighbor analysis and add additional data from the two types of files. Then I will raise another PR to improve the current implementation 😄

Let me know if anything more needs to be done before it can be merged :-)

…nto structure_graphs pull from remote

…nto structure_graphs fetch latest remote changes

…nto structure_graphs sync remote

naik-aakash · 2023-09-14T06:09:37Z

Hi @JaGeo , here as well issues seem to be resolved so this could be merged

naik-aakash · 2023-09-14T06:14:30Z

There are still conflicts, unfortunately :(

Just resolved it now , should be fine. test files xD

naik-aakash added 4 commits August 23, 2022 16:18

initial rough draft

6a66c87

add structure_graph module

eb45aa7

removed unwanted comments

ad74c3c

minor changes

91e1548

added ICOHP bonding-antibonding properties

b051f1b

JaGeo requested changes Aug 25, 2022

View reviewed changes

naik-aakash added 5 commits August 25, 2022 12:44

removed redundant methods, renamed graphobject class,option to switch…

0278753

… all/cation-anion bond

fix linting

35dd472

rename "ICOHP_bond_key" to "bond_label"

6b2cec3

added test case

6359234

removed blank space

04a4096

naik-aakash added the enhancement New feature or request label Aug 26, 2022

JaGeo self-requested a review August 27, 2022 09:41

Merge branch 'main' into structure_graphs

8dff9a2

Merge branch 'main' into structure_graphs

cc9d6e7

JaGeo reviewed Nov 1, 2022

View reviewed changes

naik-aakash and others added 6 commits November 1, 2022 17:49

Update graph.py

074c871

Added comments

Update graph.py

7d45f13

Merge branch 'JaGeo:main' into structure_graphs

7885eae

update doc strings

613da7b

doc strings

9cc12cf

fix add_additional_data_sg not initialized

7dfa7ba

naik-aakash added 2 commits April 25, 2023 08:00

Merge branch 'JaGeo:main' into structure_graphs

7f60606

Merge branch 'JaGeo:main' into structure_graphs

33fda06

add missing paramters to lobsterneighbors

30bdf25

resolve merge conflicts

8dcdb44

JaGeo requested changes Sep 13, 2023

View reviewed changes

naik-aakash and others added 4 commits September 13, 2023 09:07

Merge branch 'JaGeo:main' into structure_graphs

3fa39f5

Update docs/source/conf.py

017c50e

Co-authored-by: J. George <JaGeo@users.noreply.github.com>

Update docs/source/conf.py

d3c193f

Co-authored-by: J. George <JaGeo@users.noreply.github.com>

add more description to Lobstergraph class

c0f3c56

naik-aakash and others added 3 commits September 13, 2023 09:57

fix typo

deab80a

Merge branch 'JaGeo:main' into structure_graphs

283dfa0

reolve merge conflicts

466b9d8

JaGeo force-pushed the structure_graphs branch from 466b9d8 to deab80a Compare September 13, 2023 10:35

JaGeo and others added 11 commits September 13, 2023 12:40

Add example

e07aa20

Merge branch 'structure_graphs' of github.com:naik-aakash/LobsterPy i…

64b108a

…nto structure_graphs pull from remote

resolve merge conflict

442288e

Merge branch 'structure_graphs' of github.com:naik-aakash/LobsterPy i…

d3e1828

…nto structure_graphs fetch latest remote changes

resolve merge conflict

8045c75

fix failing test due to test file update

af44445

Merge branch 'structure_graphs' of github.com:naik-aakash/LobsterPy i…

49d3912

…nto structure_graphs sync remote

resolve conflicts

e59f506

2nd attempt to fix

6f2d8c5

run black

27142fc

fix numerical issues in test for NaCl sg

091aa42

JaGeo approved these changes Sep 14, 2023

View reviewed changes

resolve conflicts

a1383f0

JaGeo merged commit 0889f07 into JaGeo:main Sep 14, 2023
21 checks passed

naik-aakash deleted the structure_graphs branch June 20, 2024 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structure graphs #63

Structure graphs #63

naik-aakash commented Aug 24, 2022 •

edited

Loading

JaGeo commented Aug 24, 2022

naik-aakash commented Aug 24, 2022 •

edited

Loading

JaGeo left a comment •

edited

Loading

JaGeo Aug 25, 2022

naik-aakash Aug 25, 2022

naik-aakash Nov 1, 2022

JaGeo Aug 25, 2022

naik-aakash Nov 1, 2022

naik-aakash commented Aug 26, 2022 •

edited

Loading

JaGeo commented Nov 1, 2022

JaGeo Nov 1, 2022

naik-aakash Nov 1, 2022 •

edited

Loading

JaGeo Nov 1, 2022

naik-aakash Nov 1, 2022

naik-aakash Nov 1, 2022

naik-aakash commented Apr 17, 2023

JaGeo commented Apr 17, 2023

naik-aakash commented Aug 25, 2023

JaGeo left a comment

JaGeo Sep 13, 2023

naik-aakash Sep 13, 2023 •

edited

Loading

naik-aakash Sep 13, 2023

JaGeo Sep 13, 2023

JaGeo Sep 13, 2023

naik-aakash Sep 13, 2023

JaGeo Sep 13, 2023 •

edited

Loading

naik-aakash Sep 13, 2023

JaGeo Sep 13, 2023

JaGeo Sep 13, 2023

naik-aakash Sep 13, 2023

naik-aakash Sep 13, 2023

naik-aakash commented Sep 13, 2023 •

edited

Loading

naik-aakash commented Sep 14, 2023

naik-aakash commented Sep 14, 2023

Structure graphs #63

Structure graphs #63

Conversation

naik-aakash commented Aug 24, 2022 • edited Loading

JaGeo commented Aug 24, 2022

naik-aakash commented Aug 24, 2022 • edited Loading

JaGeo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naik-aakash commented Aug 26, 2022 • edited Loading

JaGeo commented Nov 1, 2022

Choose a reason for hiding this comment

naik-aakash Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naik-aakash commented Apr 17, 2023

JaGeo commented Apr 17, 2023

naik-aakash commented Aug 25, 2023

JaGeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naik-aakash Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaGeo Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naik-aakash commented Sep 13, 2023 • edited Loading

naik-aakash commented Sep 14, 2023

naik-aakash commented Sep 14, 2023

naik-aakash commented Aug 24, 2022 •

edited

Loading

naik-aakash commented Aug 24, 2022 •

edited

Loading

JaGeo left a comment •

edited

Loading

naik-aakash commented Aug 26, 2022 •

edited

Loading

naik-aakash Nov 1, 2022 •

edited

Loading

naik-aakash Sep 13, 2023 •

edited

Loading

JaGeo Sep 13, 2023 •

edited

Loading

naik-aakash commented Sep 13, 2023 •

edited

Loading