Skip to content

Init data module & send matrices to proper devices - #421

Open
MarshallYan wants to merge 2 commits into
mainfrom
feature-nvalchemi-d3-support
Open

Init data module & send matrices to proper devices#421
MarshallYan wants to merge 2 commits into
mainfrom
feature-nvalchemi-d3-support

Conversation

@MarshallYan

@MarshallYan MarshallYan commented May 19, 2026

Copy link
Copy Markdown
Collaborator

…mi-ops)

Pull Request Summary

Init modelforge/data as a module so that import can be done despite the running directory. Fix a typo of not copying a matrix to devices.

Key changes

  • Init modelforge/data as a python module so that its path can be imported
  • Copy interact_fillers to proper devices (CPU or GPU), this was a typo from last nvalchemi-ops dispersion calculation

Associated Issue(s)

Pull Request Checklist

  • Issue(s) raised/addressed and linked
  • Includes appropriate unit test(s)
  • Appropriate docstring(s) added/updated
  • Appropriate .rst doc file(s) added/updated
  • PR is ready for review

@codecov-commenter

codecov-commenter commented May 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.70%. Comparing base (7be5b60) to head (db8cfd4).
⚠️ Report is 21 commits behind head on main.

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

@MarshallYan
MarshallYan enabled auto-merge May 20, 2026 17:43

@chrisiacovella chrisiacovella 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.

Trying to figure out what was changed to address the nvalchemi import issue.

"""

"""
r"""

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.

Is this just a stray character?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The 'r' indicates that the doc string is a raw string since there are backward slashes in it and this might confuse the Python interpreter.

Raw strings explained.


def __iter__(self) -> Iterator[Dict[str, float]]:
"""Iterate over the energies dictionary."""
from modelforge.utils.units import chem_context

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.

This needs to be imported somewhere in the file, otherwise we can't use the appropriate conversions of energy (well energy to energy/mol).

this imports the context required for openff units that is called, e.g., on line 166

.to(GlobalUnitSystem.get_units("energy"), "chem")

@chrisiacovella

Copy link
Copy Markdown
Member

Ah, I missed that you added in the init_.py file.

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.

3 participants