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

Add missing package error and unit tests for molecule interfaces #150

Merged
merged 13 commits into from
Sep 25, 2024

Conversation

dormrod
Copy link
Contributor

@dormrod dormrod commented Sep 19, 2024

Description

The functional changes in this PR focus on the behaviour of PLAMS with optional packages / behaviour.

Outside of the required packages, some of the results management in PLAMS require optional packages like ASE/RDKit etc. which need to be installed separately.

Previously there were a few ways this was handled:

  • No handling - raise ImportError when the function is called. Not particularly elegant.
  • Remove functions from __all__ if it the package could not be found (rdkit). This to me is confusing as the user would just get ImportError: cannot import name 'from_smiles' from 'scm.plams' which is confusing as this method is documented.

The solution proposed here is to raise a new error MissingOptionalPackageError when one of the results methods is called. This is added to a method through the @requires_optional_package decorator. This message is more informative.

In addition, add/refactor a bunch of unit tests for:

  • Decorators
  • Identify
  • Molecule interfaces (rdkit, ase, Packmol)

N.B. can also add this in for UCS support

@dormrod dormrod changed the title Add unit tests for molecule interfaces Add missing package error and unit tests for molecule interfaces Sep 19, 2024
@dormrod dormrod force-pushed the DavidOrmrodMorley/mol_interfaces branch 5 times, most recently from 891b12b to 0b6d7a4 Compare September 20, 2024 07:52
@dormrod dormrod force-pushed the DavidOrmrodMorley/mol_interfaces branch from 709c297 to 10e344a Compare September 24, 2024 09:58
Copy link
Member

@robertrueger robertrueger left a comment

Choose a reason for hiding this comment

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

Good idea with the custom exception for missing optional packages. Much superior to what we had before.

@dormrod dormrod merged commit ee44ad4 into trunk Sep 25, 2024
14 checks passed
@dormrod dormrod deleted the DavidOrmrodMorley/mol_interfaces branch September 25, 2024 14:20
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