Skip to content
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

Add high-level positions getter, exclude virtual sites by default #1074

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Oct 7, 2024

Description

While working on a short-term solution to #1054, I realized there's no clean high-level getter in the public API for accessing positions with virtual sites. This adds that, via refactoring to_openmm_positions, and documents some of the quirks in the existing design around positions and other topology objects.

Checklist

  • Add Interchange.get_positions with (optionally, off by default) virtual sites
  • Refactor to_openmm_positions to use the non-OpenMM positions getter
  • Document quirks
  • Add tests
  • Lint
  • Update docstrings

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.59%. Comparing base (63756b7) to head (1bf72b4).
Report is 4 commits behind head on develop.

Additional details and impacted files

@mattwthompson mattwthompson changed the title Add high-level positions getter, exclude virtual site by default Add high-level positions getter, exclude virtual sites by default Oct 8, 2024
@mattwthompson mattwthompson marked this pull request as ready for review October 8, 2024 12:45
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

LGTM! The code looks good, and I think this makes a lot of sense for the API too.

@mattwthompson
Copy link
Member Author

Thanks!

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