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

GitHub actions ci #198

Merged
merged 25 commits into from
Jan 21, 2021
Merged

GitHub actions ci #198

merged 25 commits into from
Jan 21, 2021

Conversation

leeping
Copy link
Owner

@leeping leeping commented Jan 20, 2021

From here: mattwthompson#1

This PR also fixes an issue with TINKER version parsing.

@leeping leeping requested a review from j-wags January 21, 2021 02:44
Copy link
Collaborator

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Added a few suggestions. @leeping I see there's also a change for a different bug -- Could you document that in the PR body text?

I think this is in a good place to merge, but it'll be easy to lose track of the remaining to-dos, so three issues should be opened first:

  • EITHER putting the OE license back, OR changing the little remaining functionality that requires it over to SDF (should be possible now that OFFTK supports partial-charges-in-SDF). I'd think that the latter is a better choice since it will remove the need for an OE license in CI and will let @leeping allow PRs from forks.
  • Re-enabling Py27 tests
  • Re-establishing double precision GROMACS builds

- name: Install GROMACS
shell: bash -l {0}
run: |
# This will not install double precision, needs to be replaced with a fresh build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's easy to lose the context of these sorts of comments

Suggested change
# This will not install double precision, needs to be replaced with a fresh build
# TODO: As a result of the move from Travis to GitHub Actions, the CI no longer builds
# GROMACS from source with double precision. This will need to be reverted so
# that the bromine test can return to its original strictness.

@@ -360,7 +360,7 @@ def calltinker(self, command, stdin=None, print_to_screen=False, print_command=F
if len(vw.split('.')) <= 2:
vn = float(vw)
else:
vn = float(vw.split('.')[:2])
vn = float(vw.split('.')[0]) + 0.1*float(vw.split('.')[1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a fix for the known bug that @leeping mentioned in the slack chat. @leeping, to ensure that there's a paper trail, could you edit the PR description to include something like

- [x] Fixes <description of issue here>

Copy link
Owner Author

Choose a reason for hiding this comment

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

This issue was reported by a student in a private message but I don't think there's an issue corresponding to it.

- macOS-latest
- ubuntu-latest
python-version:
- 3.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- 3.6
# TODO: Python 2.7 tests were lost during migration from Travis to Github Actions
- 3.6

@leeping
Copy link
Owner Author

leeping commented Jan 21, 2021

I've created an issue for all three items in #199 . Thanks a lot for your help with the review! :)

@leeping leeping merged commit 0605845 into master Jan 21, 2021
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