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.

Sign up for GitHub

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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
644c12d
oops wrong branch
DanielYang59 Nov 25, 2024
87da226
test __eq__
DanielYang59 Nov 26, 2024
bc6645b
fix typo
DanielYang59 Nov 26, 2024
5b4b0ae
add test for illegal mode
DanielYang59 Nov 26, 2024
c2ccd92
minor code clean up
DanielYang59 Nov 26, 2024
13e58d6
add test
DanielYang59 Nov 26, 2024
dd1b338
add test for non-numerical warning
DanielYang59 Nov 26, 2024
2289d54
enhance dostring
DanielYang59 Nov 26, 2024
9618ddc
more descriptive var name
DanielYang59 Nov 26, 2024
467b840
use pytest skipif mark
DanielYang59 Nov 26, 2024
658b33a
mark classvar
DanielYang59 Nov 26, 2024
99988ef
remove skip mark altogether, no reason to check test file
DanielYang59 Nov 26, 2024
702b80b
add type alias type
DanielYang59 Nov 26, 2024
091c911
NEED CONFIRM: make properties instance var
DanielYang59 Nov 26, 2024
ecc2d05
Merge branch 'master' into fix-stru-prop-check
DanielYang59 Dec 12, 2024
78e880b
ruff fix
DanielYang59 Dec 12, 2024
8c1fb79
Merge branch 'master' into fix-stru-prop-check
DanielYang59 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pymatgen/core/structure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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]

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

if props[key] is not None and np.array_equal(self[i].properties[key], props[key]):
if mode.lower()[0] == "a" and isinstance(props[key], float):
Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

# update a running total
props[key] = props[key] * (n + 1) / (n + 2) + self[i].properties[key] / (n + 2)
Expand Down
Loading