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

Switch to base toolkit package #38

Closed
wants to merge 1 commit into from
Closed

Switch to base toolkit package #38

wants to merge 1 commit into from

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jun 7, 2023

I'm not necessarily sure this should be merged; the objective here is to see what happens in tests if NAGL is provided openff-toolkit-base and if anything is missing and needs to be packaged.

Changes made in this Pull Request:

  • Switch to using openff-toolkit-base

PR Checklist

  • Switch to using openff-toolkit-base
  • Add back any dependencies not brought in by openff-toolkit-base
  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@mattwthompson mattwthompson marked this pull request as draft June 7, 2023 19:37
@mattwthompson
Copy link
Member Author

Surprisingly only people like me who don't think things through, the tests here fail because AmberTools is needed for AM1-BCC charges when openeye=false. I'd like to be able to hand-wave this away and argue it should remain a test-only dependency. That doesn't seem satisfactory, though, since code like

MoleculeRecord.from_openff(
            offmol,
            generate_conformers=True,
            partial_charge_methods=["am1bcc"],
            bond_order_methods=["am1"],
        )

bubbles up with the expected ValueError: No registered toolkits can provide the capability "assign_partial_charges" .... Is this viewed as an acceptable outcome? I mean, if the user doesn't have something installed that provides AM1-BCC, I don't know how they'd expect a method named MoleculeRecord.from_openff to work. I don't know the code well enough to asses if MoleculeRecord is core functionality or not. Another option here would be to catch and handle this case.

@lilyminium
Copy link
Collaborator

MoleculeRecordwas core functionality, but will stop being so in 0.3.x (everything in storage is going away) and the replacement will hopefully be clearer in that you need to have a charge method available -- the replacement will be something like openff.nagl.training._loss.ChargeReadout.compute_reference. That being said, I think you could do MoleculeRecord.from_openff(partial_charge_methods=[]) currently and it would still work -- so I'm happy for the user to try doing something am1 related and bump into this error if they don't have requisite toolkitwrappers enabled.

Would it be alright if I put this temporarily on hold until I push more into https://github.com/openforcefield/openff-nagl/tree/sqlite-to-pyarrow?

@mattwthompson
Copy link
Member Author

Happy to pick this up whenever those changes are pushed through

@lilyminium
Copy link
Collaborator

Closing as superseded by #45.

@lilyminium lilyminium closed this Aug 9, 2023
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