Skip to content

Fix silent float32 precision truncation in MatterSim graph construction - #788

Open
ellalbolland wants to merge 1 commit into
ddmms:mainfrom
ellalbolland:fix-mattersim-precision-truncation
Open

Fix silent float32 precision truncation in MatterSim graph construction#788
ellalbolland wants to merge 1 commit into
ddmms:mainfrom
ellalbolland:fix-mattersim-precision-truncation

Conversation

@ellalbolland

Copy link
Copy Markdown
Contributor

Pre-review checklist for PR author

PR author must check the checkboxes below when creating the PR.

  • I've confirmed the contribution guidelines.
  • I have reviewed and understand all AI-generated code in this PR.
  • I have added human-written tests for the new logic.
  • I have properly cited any upstream algorithms or libraries the AI utilized.
  • I have disclosed significant AI tool usage in the PR description.

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)

@ElliottKasoar ElliottKasoar added the bug Something isn't working label Aug 4, 2026
@ElliottKasoar

Copy link
Copy Markdown
Collaborator

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)?

@ellalbolland

Copy link
Copy Markdown
Contributor Author

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.

@ElliottKasoar

Copy link
Copy Markdown
Collaborator

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.

@joehart2001

Copy link
Copy Markdown
Collaborator

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

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix silent float32 precision truncation in MatterSim graph construction

3 participants