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 HIP platform to openmmtools #753

Merged
merged 7 commits into from
Nov 14, 2024
Merged

add HIP platform to openmmtools #753

merged 7 commits into from
Nov 14, 2024

Conversation

mikemhenry
Copy link
Contributor

Description

Provide a brief description of the PR's purpose here.

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog message


Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.99%. Comparing base (8352b38) to head (1ecbae8).
Report is 1 commits behind head on main.

Additional details and impacted files

@mikemhenry mikemhenry mentioned this pull request Oct 8, 2024
2 tasks
@mikemhenry mikemhenry marked this pull request as ready for review October 23, 2024 22:37
@mikemhenry mikemhenry requested a review from ijpulidos October 23, 2024 22:37
Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

These looks good to me. We do need to add tests for the HIP to our GPU CI, but I don't think that's a blocker now. It's a good idea to let users start playing with this platform, imo. Thanks!

@ijpulidos
Copy link
Contributor

Since we have the openmm 8.2.0 rc in conda-forge should we test this PR with that rc? At least temporarily to run the CI with it. Maybe we actually need a workflow that runs CI with rc in general.

@mikemhenry
Copy link
Contributor Author

mikemhenry commented Nov 13, 2024

Now that 8.2 is out, we can just double check things pass and 8.2 is pulled in

EDIT: Forgot we are explicit about the openmm versions we test

@mikemhenry
Copy link
Contributor Author

I've added 8.2.0 to the testing matrix and removed older openmm versions.

Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

We need to write the tests to properly validate the platform but this is a great starting point.

@mikemhenry mikemhenry merged commit cba4ed5 into main Nov 14, 2024
22 checks passed
@mikemhenry mikemhenry deleted the feat/add-hip-platform branch November 14, 2024 23:05
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