Fix array comparison in `core.Structure.merge_sites`, also allow `int` property to be merged instead of `float` alone, `mode` only allow full name #4198

DanielYang59 · 2024-11-25T11:06:50Z

Summary

Fix array comparison in core.Structure.merge_sites, to fix Structure.merge_sites fails when selective_dynamics is property present #4197
Test for property reset warning is missing
Add test for is_np_dict_equal for custom __eq__
Add check for illegal mode initials (not starting with a/s/d, currently don't check full name yet)

Warning

Minor behaviour change (fix) in merge_sites property handling

Only allowing float to be averaged may not be a good idea, we should include int as well, otherwise properties={"prop1": 100} would lead to the site property being reset to None while properties={"prop1": 100.0} doesn't, which is super confusing

pymatgen/tests/core/test_structure.py

Lines 1677 to 1679 in 31f1e1f

    
           navs2 = Structure.from_spacegroup(160, lattice, species, coords, site_properties=site_props) 
        
           navs2.insert(0, "Na", coords[0], properties={"prop1": 100.0}) 
        
           navs2.merge_sites(mode="a")

Also the docstring is clarified to explain this behaviour.

Warning

mode only allow "full name" instead of initials after an one-year deprecation period

I believe it's beneficial to allow only full name ("sum", "delete", "average") instead of checking first letter only:

more readable: mode="sum" instead of mode="s"
consistent with currently type annotation:

pymatgen/src/pymatgen/core/structure.py

Line 4719 in 31f1e1f

def merge_sites(self, tol: float = 0.01, mode: Literal["sum", "delete", "average"] = "sum") -> Self:

DanielYang59 · 2024-11-25T11:30:25Z

src/pymatgen/core/structure.py

@@ -4746,7 +4746,7 @@ def merge_sites(self, tol: float = 0.01, mode: Literal["sum", "delete", "average
                offset = self[i].frac_coords - coords
                coords += ((offset - np.round(offset)) / (n + 2)).astype(coords.dtype)
                for key in props:
-                    if props[key] is not None and self[i].properties[key] != props[key]:


TODO tag: Using array_equal may not be a good idea either as the property could be (sequence of) floats

TODO2: test failure

pymatgen/tests/core/test_structure.py

Lines 1667 to 1681 in 31f1e1f

# Test that we can average the site properties that are floats

lattice = Lattice.hexagonal(3.587776, 19.622793)

species = ["Na", "V", "S", "S"]

coords = [

[0.333333, 0.666667, 0.165000],

[0, 0, 0.998333],

[0.333333, 0.666667, 0.399394],

[0.666667, 0.333333, 0.597273],

]

site_props = {"prop1": [3.0, 5.0, 7.0, 11.0]}

navs2 = Structure.from_spacegroup(160, lattice, species, coords, site_properties=site_props)

navs2.insert(0, "Na", coords[0], properties={"prop1": 100.0})

navs2.merge_sites(mode="a")

assert len(navs2) == 12

assert 51.5 in [itr.properties["prop1"] for itr in navs2]

Property can actually be anything, including value supplied by user.

thanks for stepping in, I'm here adding a note for myself to work on later.

Yes in this case using == for comparison may not be ideal when the value could be float or np.array, this is very similar to #4092 where we want to compare the equality of two dict whose value could be np.array

But I assume we would not be able to use np.allclose as the value could be non-numerical, so as far as I'm aware, using array_equal would be the best approach (though handling of float would be sub-optimal)

DanielYang59 · 2024-11-26T10:53:15Z

src/pymatgen/core/structure.py

-        dist_mat = self.distance_matrix
+        # TODO: change the code the allow full name after 2025-12-01
+        # TODO2: add a test for mode value, currently it only checks if first letter is "s/a"
+        if mode.lower() not in {"sum", "delete", "average"} and mode.lower()[0] in {"s", "d", "a"}:


Want to hear more opinions on this, I guess it's beneficial to allow only full name ("sum", "delete", "average") instead of checking first letter only to make use of the IDE auto-complete feature and facilitate typing

Also using the full name would be more readable: mode="sum" instead of mode="s"

DanielYang59 · 2024-11-26T11:09:15Z

src/pymatgen/core/structure.py

            for n, i in enumerate(inds[1:]):
                sp = self[i].species
                if mode.lower()[0] == "s":
                    species += sp
                offset = self[i].frac_coords - coords
                coords += ((offset - np.round(offset)) / (n + 2)).astype(coords.dtype)
                for key in props:
-                    if props[key] is not None and self[i].properties[key] != props[key]:
+                    if props[key] is not None and not np.array_equal(self[i].properties[key], props[key]):
                        if mode.lower()[0] == "a" and isinstance(props[key], float):


I guess only allowing float to be averaged may not be a good idea, we should include int as well, otherwise properties={"prop1": 100} would lead to the site property being reset to None while properties={"prop1": 100.0} doesn't, which is super confusing

pymatgen/tests/core/test_structure.py

Lines 1677 to 1679 in 31f1e1f

navs2 = Structure.from_spacegroup(160, lattice, species, coords, site_properties=site_props)

navs2.insert(0, "Na", coords[0], properties={"prop1": 100.0})

navs2.merge_sites(mode="a")

Also the docstring would be clarified to explain this behaviour.

DanielYang59 · 2024-11-26T13:42:43Z

src/pymatgen/core/structure.py

@@ -216,7 +216,7 @@ class SiteCollection(collections.abc.Sequence, ABC):
    """

    # Tolerance in Angstrom for determining if sites are too close
-    DISTANCE_TOLERANCE = 0.5
+    DISTANCE_TOLERANCE: ClassVar[float] = 0.5
    _properties: dict


NEED CONFIRM on 091c911: Is _properties misplaced? an instance variable maybe?

pymatgen/src/pymatgen/core/structure.py

Lines 211 to 220 in 31f1e1f

class SiteCollection(collections.abc.Sequence, ABC):

"""Basic SiteCollection. Essentially a sequence of Sites or PeriodicSites.

This serves as a base class for Molecule (a collection of Site, i.e., no

periodicity) and Structure (a collection of PeriodicSites, i.e.,

periodicity). Not meant to be instantiated directly.

"""

# Tolerance in Angstrom for determining if sites are too close

DISTANCE_TOLERANCE = 0.5

_properties: dict

DanielYang59 force-pushed the fix-stru-prop-check branch from 1823057 to ab07a03 Compare November 25, 2024 11:08

oops wrong branch

644c12d

DanielYang59 force-pushed the fix-stru-prop-check branch from ab07a03 to 644c12d Compare November 25, 2024 11:09

DanielYang59 commented Nov 25, 2024

View reviewed changes

DanielYang59 added 2 commits November 26, 2024 17:55

test __eq__

87da226

fix typo

bc6645b

DanielYang59 commented Nov 26, 2024

View reviewed changes

add test for illegal mode

5b4b0ae

DanielYang59 commented Nov 26, 2024

View reviewed changes

minor code clean up

c2ccd92

DanielYang59 changed the title ~~Fix array comparison in core.Structure.merge_sites~~ Fix array comparison in core.Structure.merge_sites, merge_sites also allow int property to be merged instead of float alone, mode only allow full name Nov 26, 2024

DanielYang59 added 2 commits November 26, 2024 20:41

add test

13e58d6

add test for non-numerical warning

dd1b338

DanielYang59 force-pushed the fix-stru-prop-check branch from b26b299 to dd1b338 Compare November 26, 2024 12:44

DanielYang59 added 5 commits November 26, 2024 20:50

enhance dostring

2289d54

more descriptive var name

9618ddc

use pytest skipif mark

467b840

mark classvar

658b33a

remove skip mark altogether, no reason to check test file

99988ef

DanielYang59 marked this pull request as ready for review November 26, 2024 13:37

DanielYang59 requested review from shyuep and mkhorton as code owners November 26, 2024 13:37

add type alias type

702b80b

DanielYang59 commented Nov 26, 2024

View reviewed changes

NEED CONFIRM: make properties instance var

091c911

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix array comparison in `core.Structure.merge_sites`, also allow `int` property to be merged instead of `float` alone, `mode` only allow full name #4198

Fix array comparison in `core.Structure.merge_sites`, also allow `int` property to be merged instead of `float` alone, `mode` only allow full name #4198

DanielYang59 commented Nov 25, 2024 •

edited

Loading

DanielYang59 Nov 25, 2024 •

edited

Loading

yantar92 Nov 25, 2024

DanielYang59 Nov 25, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading

	navs2 = Structure.from_spacegroup(160, lattice, species, coords, site_properties=site_props)
	navs2.insert(0, "Na", coords[0], properties={"prop1": 100.0})
	navs2.merge_sites(mode="a")

	# Test that we can average the site properties that are floats
	lattice = Lattice.hexagonal(3.587776, 19.622793)
	species = ["Na", "V", "S", "S"]
	coords = [
	[0.333333, 0.666667, 0.165000],
	[0, 0, 0.998333],
	[0.333333, 0.666667, 0.399394],
	[0.666667, 0.333333, 0.597273],
	]
	site_props = {"prop1": [3.0, 5.0, 7.0, 11.0]}
	navs2 = Structure.from_spacegroup(160, lattice, species, coords, site_properties=site_props)
	navs2.insert(0, "Na", coords[0], properties={"prop1": 100.0})
	navs2.merge_sites(mode="a")
	assert len(navs2) == 12
	assert 51.5 in [itr.properties["prop1"] for itr in navs2]

	class SiteCollection(collections.abc.Sequence, ABC):
	"""Basic SiteCollection. Essentially a sequence of Sites or PeriodicSites.
	This serves as a base class for Molecule (a collection of Site, i.e., no
	periodicity) and Structure (a collection of PeriodicSites, i.e.,
	periodicity). Not meant to be instantiated directly.
	"""

	# Tolerance in Angstrom for determining if sites are too close
	DISTANCE_TOLERANCE = 0.5
	_properties: dict

Fix array comparison in core.Structure.merge_sites, also allow int property to be merged instead of float alone, mode only allow full name #4198

Are you sure you want to change the base?

Fix array comparison in core.Structure.merge_sites, also allow int property to be merged instead of float alone, mode only allow full name #4198

Conversation

DanielYang59 commented Nov 25, 2024 • edited Loading

Summary

DanielYang59 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

yantar92 Nov 25, 2024

Choose a reason for hiding this comment

DanielYang59 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

DanielYang59 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

DanielYang59 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

DanielYang59 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Fix array comparison in `core.Structure.merge_sites`, also allow `int` property to be merged instead of `float` alone, `mode` only allow full name #4198

Fix array comparison in `core.Structure.merge_sites`, also allow `int` property to be merged instead of `float` alone, `mode` only allow full name #4198

DanielYang59 commented Nov 25, 2024 •

edited

Loading

DanielYang59 Nov 25, 2024 •

edited

Loading

DanielYang59 Nov 25, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading

DanielYang59 Nov 26, 2024 •

edited

Loading