Skip to content

Fix unstable entries sorting in PhaseDiagram causing intermittent CI failure#4584

Merged
shyuep merged 1 commit intomaterialsproject:masterfrom
DanielYang59:fix-test-equal-compare
Feb 18, 2026
Merged

Fix unstable entries sorting in PhaseDiagram causing intermittent CI failure#4584
shyuep merged 1 commit intomaterialsproject:masterfrom
DanielYang59:fix-test-equal-compare

Conversation

@DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Feb 8, 2026

Fix currently intermittent CI failure, cc @Andrew-S-Rosen :

    def test_as_from_dict(self):
        # test round-trip for other entry types such as ComputedEntry
        entry = ComputedEntry("H", 0.0, 0.0, entry_id="test")
        pd = PhaseDiagram([entry])
        pd_dict = pd.as_dict()
        pd_roundtrip = PhaseDiagram.from_dict(pd_dict)
        assert pd.all_entries[0].entry_id == pd_roundtrip.all_entries[0].entry_id
        dd = self.pd.as_dict()
        new_pd = PhaseDiagram.from_dict(dd)
        new_pd_dict = new_pd.as_dict()
        assert len(new_pd_dict) == len(dd)
        for k, v in new_pd_dict.items():
>           assert v == dd[k]
E           AssertionError: assert {'all_entries...2), ...], ...} == {'all_entries...2), ...], ...}
E             
E             (pytest_assertion plugin: representation of details failed: /Users/runner/micromamba/envs/pmg/lib/python3.13/site-packages/_pytest/assertion/util.py:494: ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all().
E              Probably an object has a faulty __repr__.)

I tried to extract the mismatch:

E                       AssertionError: Mismatch in computed_data['qhull_entries']

E                         - 4, 296, 302, 308, 31
E                         ?           ^
E                         + 4, 296, 300, 308, 31
E                         ? 

Looks like when entries have the same composition, their sorting sort might be unstable

pre-commit auto-fixes
@DanielYang59 DanielYang59 force-pushed the fix-test-equal-compare branch from 58e6e41 to faa4466 Compare February 18, 2026 17:42
@DanielYang59
Copy link
Contributor Author

@shyuep I'm still seeing intermittent CI failure because of this issue, maybe you could review this? thanks

all_entries: list[PDEntry] = []
for composition, group_iter in itertools.groupby(entries, key=lambda e: e.composition.reduced_composition):
group = list(group_iter)
min_entry = min(group, key=lambda e: e.energy_per_atom)
Copy link
Member

Choose a reason for hiding this comment

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

If you already sorted by composition and energy, then this need not use min any more? You can just do group[0]?

@shyuep shyuep merged commit 5c43b4a into materialsproject:master Feb 18, 2026
44 checks passed
@shyuep
Copy link
Member

shyuep commented Feb 18, 2026

Nvm, i will fix it on my side. Thanks.

@DanielYang59
Copy link
Contributor Author

thanks that's a good point, i was about to push but it has been merged ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants