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

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


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