Skip to content

feat: Add support for reading unit cell dimensions from MOL2 CRYSIN records - #5187

Merged
orbeckst merged 18 commits into
MDAnalysis:developfrom
Dreamstick9:feat/3341-add-crysin-support
Mar 13, 2026
Merged

feat: Add support for reading unit cell dimensions from MOL2 CRYSIN records#5187
orbeckst merged 18 commits into
MDAnalysis:developfrom
Dreamstick9:feat/3341-add-crysin-support

Conversation

@Dreamstick9

@Dreamstick9 Dreamstick9 commented Dec 23, 2025

Copy link
Copy Markdown
Contributor

Fixes #3341

Changes made in this Pull Request:

Implemented logic in MOL2Parser (package/MDAnalysis/coordinates/MOL2.py) to parse @CRYSIN records.

Extracted unit cell dimensions (lengths and angles) from the CRYSIN block and assigned them to ts.dimensions.

Added a new test data file with a CRYSIN record: testsuite/MDAnalysisTests/data/mol2/test_crysin.mol2.

Registered the new test file in testsuite/MDAnalysisTests/datafiles.py.

Added a regression test test_mol2_crysin_dimensions in testsuite/MDAnalysisTests/coordinates/test_mol2.py to verify dimensions are loaded correctly.

PR Checklist
[x] Issue raised/referenced?

[x] Tests updated/added?

[ ] Documentation updated/added?

[x] package/CHANGELOG file updated?

[x] Is your name in package/AUTHORS? (If it is not, add it!)

Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.


Documentation preview : https://mdanalysis--5187.org.readthedocs.build/en/5187/

@Dreamstick9
Dreamstick9 marked this pull request as draft December 23, 2025 16:05
@codecov

codecov Bot commented Dec 23, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.82%. Comparing base (6a1befe) to head (2effc53).
⚠️ Report is 40 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5187      +/-   ##
===========================================
- Coverage    93.83%   93.82%   -0.01%     
===========================================
  Files          182      182              
  Lines        22483    22487       +4     
  Branches      3195     3196       +1     
===========================================
+ Hits         21097    21099       +2     
- Misses         924      925       +1     
- Partials       462      463       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dreamstick9
Dreamstick9 marked this pull request as ready for review December 24, 2025 03:50
@orbeckst

Copy link
Copy Markdown
Member

@Dreamstick9 can you link to the document that describes the @CRYSIN record? Can you show/link to examples mol2 files found "in the wild" that use it, e.g., from a database?

That may help with reviewing.

Also make sure that you address all points in the checklist (eg, add a note to the docs, add an entry to CHANGELOG). Reviewing PRs takes a lot of time and time is really precious so show reviewers that you've done all the necessary work.

@Dreamstick9

Copy link
Copy Markdown
Contributor Author

I implemented this based on the official Tripos mol2 file format specification, the @crysin record is defined the the standard manual in page 17 of the pdf(https://github.com/UnixJunkie/mol2-file-format-spec/blob/master/mol2.pdf). the test case added in mol2_crysin was for this specification. this record is the standard method for defining periodic boundary conditions in the format (see "Data Record Format" in the spec).
i have updated the CHANGELOG and AUTHORS.

Thanks for the review @orbeckst

@Dreamstick9
Dreamstick9 force-pushed the feat/3341-add-crysin-support branch from 378627a to edfa9c3 Compare January 16, 2026 13:06
@orbeckst orbeckst self-assigned this Jan 16, 2026

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some inline comments/questions — please address those.

Update the docs where it says that MOL2 ignores CRYSIN to say that it ignores the spacegroup and settings. The unitcell is interpreted in the common crystallographic convention with box vector a parallel to X, b in the XY plane, and c with a positive Z component.

Comment thread package/CHANGELOG Outdated
Comment thread package/MDAnalysis/coordinates/MOL2.py Outdated
Comment thread package/MDAnalysis/coordinates/MOL2.py Outdated
@Dreamstick9
Dreamstick9 force-pushed the feat/3341-add-crysin-support branch from a4c6bb1 to b3839b5 Compare January 20, 2026 10:56
@Dreamstick9

Copy link
Copy Markdown
Contributor Author

Thanks for the review @orbeckst, I've made the requested changes and also removed the try and except,

@Dreamstick9
Dreamstick9 requested a review from orbeckst February 16, 2026 16:58
Comment thread package/CHANGELOG Outdated
Comment thread testsuite/MDAnalysisTests/coordinates/test_mol2.py Outdated
@Dreamstick9

Dreamstick9 commented Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

@tylerjereddy i've updated the CHANGELOG and also switched to assert_allclose in test_mol2.py
Thanks for the review!

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for addressing my comments. I have two changes that I am going to make and then it's good to go from my end.

Comment thread package/MDAnalysis/coordinates/MOL2.py Outdated
Comment thread package/MDAnalysis/coordinates/MOL2.py Outdated
@orbeckst
orbeckst merged commit 50dc512 into MDAnalysis:develop Mar 13, 2026
23 of 24 checks passed
@orbeckst

Copy link
Copy Markdown
Member

Congratulations @Dreamstick9 , your contribution is merged 🎉 ! Thank you!

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.

Add support for @<TRIPOS>CRYSIN MOL2 records

3 participants