Skip to content

add HIP platform to openmmtools#753

Merged
mikemhenry merged 7 commits into
mainfrom
feat/add-hip-platform
Nov 14, 2024
Merged

add HIP platform to openmmtools#753
mikemhenry merged 7 commits into
mainfrom
feat/add-hip-platform

Conversation

@mikemhenry

@mikemhenry mikemhenry commented Oct 3, 2024

Copy link
Copy Markdown
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

@codecov

codecov Bot commented Oct 3, 2024 •
edited
Loading

Copy link
Copy Markdown

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 5 commits behind head on main.

Additional details and impacted files

@mikemhenry mikemhenry mentioned this pull request Oct 8, 2024
Draft
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
ijpulidos
ijpulidos approved these changes Oct 23, 2024
View reviewed changes

@ijpulidos ijpulidos left a comment

Copy link
Copy Markdown
Contributor

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

ijpulidos commented Nov 6, 2024

Copy link
Copy Markdown
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

mikemhenry commented Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

mikemhenry commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

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

ijpulidos
ijpulidos approved these changes Nov 14, 2024
View reviewed changes

@ijpulidos ijpulidos left a comment

Copy link
Copy Markdown
Contributor

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

Reviewers

@ijpulidos ijpulidos ijpulidos approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mikemhenry @ijpulidos