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

Back-porting PRs to 0.8.4 branch #856

Merged
merged 54 commits into from
Mar 9, 2021
Merged

Back-porting PRs to 0.8.4 branch #856

merged 54 commits into from
Mar 9, 2021

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Mar 2, 2021

mattwthompson and others added 30 commits March 1, 2021 12:22
…a will write ['canonical_isomeric_explicit_hydrogen_mapped_smiles'] in extras dict, one test included to check the roundtrip
…s will always have cmiles entry with current fix
… on line 1904, 1912 since CI tests are failing because of a different ordering even though the atoms are tagged
@mattwthompson
Copy link
Member

I have a few failing tests from the start

  • Some of the links in the README pointed to the master branch, which is now broken because permalinks do not redirect to respect the new code paths. I updated these to point to the 084-update branch.
  • z_3_hydroxy_propenal.sdf wasn't in the tree, so I added it
  • Some number of the recent PRs were missing (at least Expose ELF conformer selection through the OpenEye wrapper. #831, probably also a WBO change). These are my currently failing tests
    • openforcefield/tests/test_molecule.py::TestMolecule::test_apply_elf_conformer_selection
      • (missing method in toolkit wrappers? or did not mean to add in test?)
    • openforcefield/tests/test_toolkits.py::TestOpenEyeToolkitWrapper::test_assign_fractional_bond_orders (several combinations of ELF10 methods and molecules)
    • openforcefield/tests/test_toolkits.py::TestOpenEyeToolkitWrapper::test_assign_fractional_bond_orders_neutral_charge_mol
    • openforcefield/tests/test_toolkits.py::TestOpenEyeToolkitWrapper::test_assign_fractional_bond_orders_multi_conf

This was done with my current development environment with both OpenEye toolkits and The RDKit installed. I also ran without OpenEye toolkits and everything passed - effectively a false positive because of

@requires_openeye
class TestMolecule:

Running with The RDKit but no OpenEye toolkits produces the same failed tests as above.

The current deprecation warning seems accurate. Would it be worth it to also link to #819, here or in the release notes?

__main__:1: DeprecationWarning: Importing this package as `import openforcefield.XXX` and `from openforcefield import XXX` was marked for deprecation in version `0.8.3`. From version `0.9.0` onwards this package will need to be imported as `import openff.toolkit.XXX` and `from openff.toolkit import XXX`. See the `0.8.3` release notes for more information.

@j-wags
Copy link
Member Author

j-wags commented Mar 2, 2021

Thanks, Matt. I'm going to keep banging on this today and will ping you when it's ready for review!

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (084-rc@520bc28). Click here to learn what that means.
The diff coverage is n/a.

@j-wags
Copy link
Member Author

j-wags commented Mar 5, 2021

Don't review this yet. I think there's actually a significant test failure. https://github.com/openforcefield/openff-toolkit/runs/2036523162#step:12:50065

This is code adjacent to some of our current WBO work/bugfixes. I'm not sure that it's good to keep this as xfail. https://github.com/openforcefield/openff-toolkit/blame/master/openff/toolkit/tests/test_toolkits.py#L2930

I'll debug more tomorrow.

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Took a brief skim through the files and couldn't find anything objectionable. I'm restarting the tests; if the only failure is the stochastic one we've already discussed, LGTM

@j-wags
Copy link
Member Author

j-wags commented Mar 8, 2021

Good news❓ Nothing fails any more❓❓ Hooray❓❓❓

@j-wags j-wags merged commit c901112 into 084-rc Mar 9, 2021
@j-wags j-wags deleted the 084-update branch March 9, 2021 00:08
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.

5 participants