Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Migrate openforcefield to openff.toolkit #803

Merged
merged 11 commits into from
Jan 14, 2021
Merged

Conversation

SimonBoothroyd
Copy link
Contributor

@SimonBoothroyd SimonBoothroyd commented Jan 5, 2021

Description

This PR handles the migration over to the new openff-toolkit package name and the namespace migration such that the package will be importable as openff.toolkit rather than openforcefield.

The consensus is that this PR should be merged ready for the first release of this package on C-F, such that the C-F version will only ever contain openff-toolkit >=0.9.0, while the omnia channel will only contain the openforcefield <=0.8.2

Todos

  • Tag issue being addressed
  • Add tests
  • Update docstrings/documentation, if applicable
  • Lint codebase
  • Update changelog
  • Update the openforcefields CI dependencies. This hasn't been changed yet otherwise the CI will fail.

Notes

  • This PR updates all links from openforcefield/openforcefield/ to openforcefield/openff-toolkit as the repo will also be renamed as part of the package rename. Until the repo is renamed however the link tests are expected to fail.
  • The RTD links will not be updated / changed as there is currently not a clean way to rename a RTD site.
  • The swap_existing_ligand_parameters_with_openmmforcefields.ipynb is expected to fail tests as it relies upon the openmmforcefields package which in turn depends upon openforcefield.

Status

  • Ready to review
  • Ready to merge

@SimonBoothroyd SimonBoothroyd marked this pull request as draft January 5, 2021 11:01
@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert and fixes 4 when merging 1f72fde into a148678 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert and fixes 4 when merging 5e0f73b into a148678 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@SimonBoothroyd SimonBoothroyd marked this pull request as ready for review January 5, 2021 12:11
@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert and fixes 4 when merging 81cc2ac into a148678 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #803 (90fc5a4) into master (68a0881) will decrease coverage by 1.72%.
The diff coverage is 92.59%.

Copy link
Member

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

I spot-checked several changes and didn't find anything objectionable. This looks good to me. Thanks, @SimonBoothroyd! I approve merging this when the process begins.

Is there an overall checklist (when to make specific tags, merge this branch, recommend opening new PRs against a specific branch, make public announcements over various channels, change CI deps in other packages, add deprecation warnings, etc) I can review? Or should we start work on that?

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2021

This pull request introduces 1 alert and fixes 4 when merging ed535e9 into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2021

This pull request introduces 1 alert and fixes 4 when merging 6269894 into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2021

This pull request introduces 1 alert and fixes 4 when merging cc890be into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@lgtm-com
Copy link

lgtm-com bot commented Jan 12, 2021

This pull request introduces 1 alert and fixes 4 when merging 0688faa into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@@ -13,9 +13,11 @@ The toolkit currently covers two main areas we have committed to stably maintain
* Tools for using the [SMIRKS Native Open Force Field (SMIRNOFF) specification](https://open-forcefield-toolkit.readthedocs.io/en/latest/smirnoff.html)
* Tools for [direct chemical environment perception](https://dx.doi.org/10.1021/acs.jctc.8b00640) and manipulation

**Note**: Prior to version 0.9.0, this toolkit and its associated repository were named `openforcefield` and used different import paths. For details on this change and migration instructions, see the [release notes](https://open-forcefield-toolkit.readthedocs.io/en/latest/releasehistory.html) of version 0.9.0.
Copy link
Member

Choose a reason for hiding this comment

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

This second sentence could be dropped or changed if it doesn't end up being accurate

@lgtm-com
Copy link

lgtm-com bot commented Jan 13, 2021

This pull request introduces 1 alert and fixes 4 when merging 5665ae2 into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@lgtm-com
Copy link

lgtm-com bot commented Jan 14, 2021

This pull request introduces 1 alert and fixes 4 when merging 90fc5a4 into 68a0881 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

fixed alerts:

  • 2 for Non-callable called
  • 1 for Module is imported more than once
  • 1 for Non-iterable used in for loop

@SimonBoothroyd SimonBoothroyd merged commit de8a4a5 into master Jan 14, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants