Skip to content

Add option to evaluate unit cells symbolically #45

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.

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

janbridley
Copy link
Collaborator

@janbridley janbridley commented Mar 19, 2025

Description

Unit cells can now be build symbolically, using sympy's Rational numbers to determine the correct atomic coordinates. This reduces rounding errors for large and complicated unit cells and allows for more accurate deduplication of atoms.

Motivation and Context

As in #42 and #44, floating point errors can accumulate to result in imperfect deduplication of atoms for complex structures. The new implementation has a few changes:

  • Fractional coordinates are rounded before PBC wrapping, reducing the number of duplicate points near the edges of a cell
  • Symmetry operation expressions can be evaluated symbolically by setting the parse_mode flag of build_unit_cell to sympy. The previous implementation is accessible by setting that value to python_float
  • New sympy evaluation mode is enabled by default only if sympy is installed on the system.

Notes on the method

Symbolically evaluating every site is slow relative to the performance of the pure python implementation - however, the total runtime per crystal is on the order of tens of milliseconds and should be fast enough for most users.

Symbolic evaluation of sites is NOT guaranteed to accurately reconstruct every crystal! While parsnip works for 1102/1103 of the files in my test suite, some tuning of n_decimal_places may be required. For really degenerate systems (orthorhombic, monoclinic, or triclinic crystals with >100 atoms and sites represented with 3 or fewer decimal places), even symbolic evaluation may not be able to properly deduplicate a crystal.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality and should be merged into the breaking branch

Checklist:

  • I am familiar with the Development Guidelines
  • The changes introduced by this pull request are covered by existing or newly introduced tests.
  • I have updated the changelog and added my name to the credits.

@janbridley janbridley requested a review from joaander March 19, 2025 19:37
@janbridley janbridley marked this pull request as ready for review March 19, 2025 19:37
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

As we discussed offline, my only concern with this PR is that in the default configuration, users may now obtain different behavior for the same script and same data input depending on whether their environment contains sympy. If you find that the convenience of sympy on by default (while at the same time being an optional dependency) outweighs this concern, feel free to merge.

I realize that your testing shows that sympy only produces different output on a vanishingly small number of CIF files. But I have sympathy for the user who will inevitably end up using one of those files and spending many hours attempting to figure out why they get an error on one computer but the file parses correctly on another. This hypothetical user may have preferred a consistent error on both that prompts them to install sympy and explicitly pass parse_mode='sympy'.

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