Fix silent float32 precision truncation in MatterSim graph construction - #788
Fix silent float32 precision truncation in MatterSim graph construction#788ellalbolland wants to merge 1 commit into
Conversation
|
Thanks for this, @ellalbolland! Would you be able to rebase/resolve the conflict now that #775 is merged, and also open an issue for this with MatterSim (it'll be useful for other people, and to discuss with their devs, but also, patches are always tricky to maintain for long)? |
|
I actually don't think we should merge this. The fix only lives in ml-peg's models.py monkeypatch, not in MatterSim itself so our table numbers reflect a MatterSim that's better-behaved than what anyone gets from pip install MatterSim. Someone reproducing our test against a plain install would likely see a bigger float32-truncation error. I will open an issue for this with MatterSim maintainers about the float32 truncation so it's tracked upstream too. Happy to rebase and resolve the conflict too, but wanted to flag the above before we merge. Let me know what you think. |
I'm also a bit torn, and so far we've followed a fairly similar principle of taking packages at face value. I'd certainly agree if it were a fix that changed the underlying performance of the model, but this is just a bug in the ASE wrapper, which feels like a bit more of a grey area. Let's see how responsive they are once you open an issue. If it's fixed within a week then it's one less thing to decide. |
agreed |
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
Patches GraphConverter.convert to rebuild atom_pos/cell at the calculator's actual requested dtype, instead of mattersim's hardcoded float32. The existing upcast in calculate() happens too late to recover lost precision, silently breaking translational symmetry even at dtype="float64".
Delegates to mattersim's own unmodified convert() for all the real logic, only rebuilding the two fields that lose precision. pbc_offsets is untouched, it's small integers, exact in float32 either way.
Linked issue
Resolves #786
Testing
AI tool usage disclosure
This investigation, root-cause diagnosis, and fix were developed with assistance from Claude (Anthropic)