From 79b97f4f0b1526501af65baab0f04ea15e7d4e96 Mon Sep 17 00:00:00 2001 From: Rohith Srinivaas M Date: Fri, 4 Aug 2023 16:54:53 -0700 Subject: [PATCH 1/5] Fix isomorphic for molecular graphs --- pymatgen/analysis/graphs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pymatgen/analysis/graphs.py b/pymatgen/analysis/graphs.py index 70a3bfc2767..defdf4c3b45 100644 --- a/pymatgen/analysis/graphs.py +++ b/pymatgen/analysis/graphs.py @@ -70,7 +70,7 @@ def _isomorphic(frag1, frag2): if len(f1_nodes) != len(f2_nodes): return False f2_edges = frag2.edges() - if len(f2_edges) != len(f2_edges): + if len(f1_edges) != len(f2_edges): return False f1_comp_dict = {} f2_comp_dict = {} From f288956778100358817eedcda716269b838b25cb Mon Sep 17 00:00:00 2001 From: Janosh Riebesell Date: Fri, 4 Aug 2023 16:59:46 -0700 Subject: [PATCH 2/5] fix f1_edges undefined --- pymatgen/analysis/graphs.py | 124 +++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 58 deletions(-) diff --git a/pymatgen/analysis/graphs.py b/pymatgen/analysis/graphs.py index 8d9cad58591..f73720237a2 100644 --- a/pymatgen/analysis/graphs.py +++ b/pymatgen/analysis/graphs.py @@ -69,6 +69,7 @@ def _isomorphic(frag1, frag2): f2_nodes = frag2.nodes(data=True) if len(f1_nodes) != len(f2_nodes): return False + f1_edges = frag1.edges() f2_edges = frag2.edges() if len(f1_edges) != len(f2_edges): return False @@ -96,34 +97,38 @@ def _isomorphic(frag1, frag2): class StructureGraph(MSONable): """ - This is a class for annotating a Structure with bond information, stored in the form - of a graph. A "bond" does not necessarily have to be a chemical bond, but can store - any kind of information that connects two Sites. + This is a class for annotating a Structure with + bond information, stored in the form of a graph. A "bond" does + not necessarily have to be a chemical bond, but can store any + kind of information that connects two Sites. """ def __init__(self, structure: Structure, graph_data=None): """ - If constructing this class manually, use the with_empty_graph method or - with_local_env_strategy method (using an algorithm provided by the local_env - module, such as O'Keeffe). + If constructing this class manually, use the `with_empty_graph` + method or `with_local_env_strategy` method (using an algorithm + provided by the `local_env` module, such as O'Keeffe). - This class that contains connection information: relationships between sites - represented by a Graph structure, and an associated structure object. + This class that contains connection information: + relationships between sites represented by a Graph structure, + and an associated structure object. - This class uses the NetworkX package to store and operate on the graph itself, but - contains a lot of helper methods to make associating a graph with a given - crystallographic structure easier. + This class uses the NetworkX package to store and operate + on the graph itself, but contains a lot of helper methods + to make associating a graph with a given crystallographic + structure easier. - Use cases for this include storing bonding information, NMR J-couplings, - Heisenberg exchange parameters, etc. + Use cases for this include storing bonding information, + NMR J-couplings, Heisenberg exchange parameters, etc. - For periodic graphs, class stores information on the graph edges of what lattice - image the edge belongs to. + For periodic graphs, class stores information on the graph + edges of what lattice image the edge belongs to. - Args: - structure (Structure): Structure object to be analyzed. - graph_data (dict): Dictionary containing graph information. Not intended to be - constructed manually see as_dict method for format. + :param structure: a Structure object + + :param graph_data: dict containing graph information in + dict format (not intended to be constructed manually, + see as_dict method for format) """ if isinstance(structure, StructureGraph): # just make a copy from input @@ -132,43 +137,46 @@ def __init__(self, structure: Structure, graph_data=None): self.structure = structure self.graph = nx.readwrite.json_graph.adjacency_graph(graph_data) - # tidy up edge attr dicts, reading to/from json duplicates information - for _, _, _, dct in self.graph.edges(keys=True, data=True): + # tidy up edge attr dicts, reading to/from json duplicates + # information + for _, _, _, d in self.graph.edges(keys=True, data=True): for key in ("id", "key"): - dct.pop(key, None) - # ensure images are tuples (conversion to lists happens when serializing back - # from json), it's important images are hashable/immutable - if to_jimage := dct.get("to_jimage"): - dct["to_jimage"] = tuple(to_jimage) - if from_jimage := dct.get("from_jimage"): - dct["from_jimage"] = tuple(from_jimage) + d.pop(key, None) + # ensure images are tuples (conversion to lists happens + # when serializing back from json), it's important images + # are hashable/immutable + if "to_jimage" in d: + d["to_jimage"] = tuple(d["to_jimage"]) + if "from_jimage" in d: + d["from_jimage"] = tuple(d["from_jimage"]) @classmethod def with_empty_graph( cls, structure: Structure, - name: str = "bonds", - edge_weight_name: str | None = None, - edge_weight_units: str | None = None, - ) -> StructureGraph: + name="bonds", + edge_weight_name=None, + edge_weight_units=None, + ): """ - Constructor for an empty StructureGraph, i.e. no edges, containing only nodes corresponding - to sites in Structure. - - Args: - structure: A pymatgen Structure object. - name: Name of the graph, e.g. "bonds". - edge_weight_name: Name of the edge weights, e.g. "bond_length" or "exchange_constant". - edge_weight_units: Name of the edge weight units, e.g. "Å" or "eV". + Constructor for StructureGraph, returns a StructureGraph + object with an empty graph (no edges, only nodes defined + that correspond to Sites in Structure). - Returns: - StructureGraph: an empty graph with no edges, only nodes defined - that correspond to sites in Structure. + :param structure (Structure): + :param name (str): name of graph, e.g. "bonds" + :param edge_weight_name (str): name of edge weights, + e.g. "bond_length" or "exchange_constant" + :param edge_weight_units (str): name of edge weight units + e.g. "Å" or "eV" + :return (StructureGraph): """ if edge_weight_name and (edge_weight_units is None): raise ValueError( - "Please specify units associated with your edge weights. Can be " - "empty string if arbitrary or dimensionless." + "Please specify units associated " + "with your edge weights. Can be " + "empty string if arbitrary or " + "dimensionless." ) # construct graph with one node per site @@ -237,7 +245,7 @@ def with_edges(structure, edges): return sg @staticmethod - def with_local_env_strategy(structure, strategy, weights=False, edge_properties=False) -> StructureGraph: + def with_local_env_strategy(structure, strategy, weights=False, edge_properties=False): """ Constructor for StructureGraph, using a strategy from :class:`pymatgen.analysis.local_env`. @@ -255,7 +263,7 @@ def with_local_env_strategy(structure, strategy, weights=False, edge_properties= sg = StructureGraph.with_empty_graph(structure, name="bonds") - for idx, neighbors in enumerate(strategy.get_all_nn_info(structure)): + for n, neighbors in enumerate(strategy.get_all_nn_info(structure)): for neighbor in neighbors: # local_env will always try to add two edges # for any one bond, one from site u to site v @@ -263,7 +271,7 @@ def with_local_env_strategy(structure, strategy, weights=False, edge_properties= # harmless, so warn_duplicates=False if edge_properties: sg.add_edge( - from_index=idx, + from_index=n, from_jimage=(0, 0, 0), to_index=neighbor["site_index"], to_jimage=neighbor["image"], @@ -273,7 +281,7 @@ def with_local_env_strategy(structure, strategy, weights=False, edge_properties= ) else: sg.add_edge( - from_index=idx, + from_index=n, from_jimage=(0, 0, 0), to_index=neighbor["site_index"], to_jimage=neighbor["image"], @@ -285,8 +293,8 @@ def with_local_env_strategy(structure, strategy, weights=False, edge_properties= return sg @property - def name(self) -> str: - """Name of graph""" + def name(self): + """:return: Name of graph""" return self.graph.graph["name"] @property @@ -394,7 +402,7 @@ def add_edge( # ensure that the first non-zero jimage index is positive # assumes that at least one non-zero index is present - is_positive = next(idx for idx in to_jimage if idx != 0) > 0 + is_positive = [idx for idx in to_jimage if idx != 0][0] > 0 if not is_positive: # let's flip the jimage, @@ -676,8 +684,8 @@ def map_indices(grp): atoms = len(grp) - 1 offset = len(self.structure) - atoms - for idx in range(atoms): - grp_map[idx] = idx + offset + for i in range(atoms): + grp_map[i] = i + offset return grp_map @@ -2090,11 +2098,11 @@ def build_unique_fragments(self): comp = "".join(sorted(comp)) subgraph = nx.subgraph(graph, combination) if nx.is_connected(subgraph): - key = f"{comp} {len(subgraph.edges())}" - if key not in frag_dict: - frag_dict[key] = [copy.deepcopy(subgraph)] + mykey = comp + str(len(subgraph.edges())) + if mykey not in frag_dict: + frag_dict[mykey] = [copy.deepcopy(subgraph)] else: - frag_dict[key].append(copy.deepcopy(subgraph)) + frag_dict[mykey].append(copy.deepcopy(subgraph)) # narrow to all unique fragments using graph isomorphism unique_frag_dict = {} From 5c648e507cf05a402602fb098d4c80c09f8c777f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 5 Aug 2023 00:01:44 +0000 Subject: [PATCH 3/5] pre-commit auto-fixes --- pymatgen/analysis/graphs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pymatgen/analysis/graphs.py b/pymatgen/analysis/graphs.py index f73720237a2..55b895d2b66 100644 --- a/pymatgen/analysis/graphs.py +++ b/pymatgen/analysis/graphs.py @@ -402,7 +402,7 @@ def add_edge( # ensure that the first non-zero jimage index is positive # assumes that at least one non-zero index is present - is_positive = [idx for idx in to_jimage if idx != 0][0] > 0 + is_positive = next(idx for idx in to_jimage if idx != 0) > 0 if not is_positive: # let's flip the jimage, From 32c67fffa5de82516e46f78281565ee213377001 Mon Sep 17 00:00:00 2001 From: Janosh Riebesell Date: Fri, 4 Aug 2023 17:06:02 -0700 Subject: [PATCH 4/5] try adding test (maybe broken) --- tests/analysis/test_graphs.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/analysis/test_graphs.py b/tests/analysis/test_graphs.py index c27a02a76c3..6f64ff9a95a 100644 --- a/tests/analysis/test_graphs.py +++ b/tests/analysis/test_graphs.py @@ -855,6 +855,12 @@ def test_isomorphic(self): assert self.ethylene.isomorphic_to(eth_copy) assert not self.butadiene.isomorphic_to(self.ethylene) + # check fix in https://github.com/materialsproject/pymatgen/pull/3221 + # by setting nodes equal so we compare edges + ethylene = self.ethylene.copy() + ethylene.nodes = self.butadiene.nodes + assert not ethylene.isomorphic_to(self.butadiene) + def test_substitute(self): molecule = FunctionalGroups["methyl"] molgraph = MoleculeGraph.with_edges( From 23f4e5cc028cf406c206354833b761b7499901ae Mon Sep 17 00:00:00 2001 From: Janosh Riebesell Date: Fri, 4 Aug 2023 17:49:39 -0700 Subject: [PATCH 5/5] fix test_isomorphic --- pymatgen/analysis/graphs.py | 6 +++--- tests/analysis/test_graphs.py | 35 ++++++++++++++++------------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/pymatgen/analysis/graphs.py b/pymatgen/analysis/graphs.py index 55b895d2b66..e1d21bd4cbb 100644 --- a/pymatgen/analysis/graphs.py +++ b/pymatgen/analysis/graphs.py @@ -60,7 +60,7 @@ def _igraph_from_nxgraph(graph): return new_igraph -def _isomorphic(frag1, frag2): +def _isomorphic(frag1: nx.Graph, frag2: nx.Graph) -> bool: """ Internal function to check if two graph objects are isomorphic, using igraph if if is available and networkx if it is not. @@ -1627,7 +1627,7 @@ def with_empty_graph(cls, molecule, name="bonds", edge_weight_name=None, edge_we return cls(molecule, graph_data=graph_data) @staticmethod - def with_edges(molecule, edges): + def with_edges(molecule: Molecule, edges: dict[tuple[int, int], dict]): """ Constructor for MoleculeGraph, using pre-existing or pre-defined edges with optional edge parameters. @@ -1651,7 +1651,7 @@ def with_edges(molecule, edges): if props is not None: weight = props.pop("weight", None) if len(props.items()) == 0: - props = None + props = None # type: ignore else: weight = None diff --git a/tests/analysis/test_graphs.py b/tests/analysis/test_graphs.py index 6f64ff9a95a..b53e9ce7315 100644 --- a/tests/analysis/test_graphs.py +++ b/tests/analysis/test_graphs.py @@ -837,33 +837,30 @@ def test_find_rings(self): def test_isomorphic(self): ethyl_xyz_path = os.path.join(PymatgenTest.TEST_FILES_DIR, "graphs/ethylene.xyz") ethylene = Molecule.from_file(ethyl_xyz_path) - # switch carbons + # swap carbons ethylene[0], ethylene[1] = ethylene[1], ethylene[0] - eth_copy = MoleculeGraph.with_edges( - ethylene, - { - (0, 1): {"weight": 2}, - (1, 2): {"weight": 1}, - (1, 3): {"weight": 1}, - (0, 4): {"weight": 1}, - (0, 5): {"weight": 1}, - }, - ) + edges = { + (0, 1): {"weight": 2}, + (1, 2): {"weight": 1}, + (1, 3): {"weight": 1}, + (0, 4): {"weight": 1}, + (0, 5): {"weight": 1}, + } + + ethylene_graph = MoleculeGraph.with_edges(ethylene, edges) # If they are equal, they must also be isomorphic - eth_copy = copy.deepcopy(self.ethylene) - assert self.ethylene.isomorphic_to(eth_copy) + assert self.ethylene.isomorphic_to(ethylene_graph) assert not self.butadiene.isomorphic_to(self.ethylene) # check fix in https://github.com/materialsproject/pymatgen/pull/3221 - # by setting nodes equal so we compare edges - ethylene = self.ethylene.copy() - ethylene.nodes = self.butadiene.nodes - assert not ethylene.isomorphic_to(self.butadiene) + # by comparing graph with equal nodes but different edges + edges[(1, 4)] = {"weight": 2} + assert not self.ethylene.isomorphic_to(MoleculeGraph.with_edges(ethylene, edges)) def test_substitute(self): molecule = FunctionalGroups["methyl"] - molgraph = MoleculeGraph.with_edges( + mol_graph = MoleculeGraph.with_edges( molecule, {(0, 1): {"weight": 1}, (0, 2): {"weight": 1}, (0, 3): {"weight": 1}}, ) @@ -885,7 +882,7 @@ def test_substitute(self): # Check that MoleculeGraph input is handled properly eth_graph.substitute_group(5, molecule, MinimumDistanceNN, graph_dict=graph_dict) - eth_mg.substitute_group(5, molgraph, MinimumDistanceNN) + eth_mg.substitute_group(5, mol_graph, MinimumDistanceNN) assert eth_graph.graph.get_edge_data(5, 6)[0]["weight"] == 1.0 assert eth_mg == eth_graph