mosdef-hub / mbuild

A hierarchical, component based molecule builder
https://mbuild.mosdef.org
Other
173 stars 80 forks source link

Update mb.load() to take in pybel object #575

Closed daico007 closed 5 years ago

daico007 commented 5 years ago

Describe the behavior you would like added to mBuild Not something that needs to be done right now, but once #555 is merged, we need another PR to updatemb.load() to be able to take in and convert pybel object.

Describe the solution you'd like A clear and concise description of what you want to happen. mb.load() can take in and convert pybel object like the way it is doing with parmed and mdtraj object in #561

uppittu11 commented 5 years ago

Not sure why this issue didn't get closed with PR #561, but I want to reopen it anyway.

After the PR, we cannot use mb.load() without importing pybel, which makes mbuild very limited unless we install openbabel.

https://github.com/mosdef-hub/mbuild/blob/fac2a904cb9871d5f448833258cfa587f5e5b852/mbuild/compound.py#L73-L83

So, I think we should add openbabel to requirements.txt as a required dependency. Or we can come up with a way to not require the import, i.e something like this:

try:
     import pybel #or pybel = import_('pybel')?
     type_dict = { 
         pmd.Structure:compound.from_parmed, 
         md.Trajectory:compound.from_trajectory, 
         pybel.Molecule:compound.from_pybel, 
     } 
except ImportError: #or is it DelayImportError from mb.utils.io?
     type_dict = { 
         pmd.Structure:compound.from_parmed, 
         md.Trajectory:compound.from_trajectory, 
     } 
mattwthompson commented 5 years ago

Good catch - I prefer the second option out of principle (fewer dependencies are better) but I am fine with the first since pybel is a mature library.

ahy3nz commented 5 years ago

Both approaches seem good - add openbabel to requirements or try/except block

Another option (I don't know if this is the best way) might be to look at the metadata associated with the filename_or_object in question and compare to structure/trajectory/pybel. These approaches never actually call a function or class from these other packages, but just look at strings

type_dict = {
        "<class 'parmed.structure.Structure'>":compound.from_parmed,
        "<class 'mdtraj.core.trajectory.Trajectory'>":compound_from_trajectory,
        "<class 'pybel.Molecule'>": compound_from_pybel
        }
actual_type = str(type(filename_or_object))
converted_cmpd = type_dict[actual_type](filename_or_object) # Maybe use dict.get() for safety
module_dict = {
        'mdtraj.core.trajectory':compound_from_trajectory,
        'parmed.structure': compound_from_parmed,
        'pybel':compound_from_pybel
        }
actual_module = filename_or_object.__module__
converted_cmpd = module_dict[actual_module](filename_or_object) # maybe use dict.get() for safety
uppittu11 commented 5 years ago

I'll submit a PR for this in a little bit. I'm going to go with the try/except block that I had suggested, and we can discuss/modify the package requirements in a later PR.

@ahy3nz This is a good option, but we might run into a bug that's hard to identify if (for whatever reason) MDTraj, Parmed, or Openbabel decide to change their __repr__() or their file organization