search

materialsproject / pymatgen

Python Materials Genomics (pymatgen) is a robust materials analysis code that defines classes for structures and molecules with support for many electronic structure codes. It powers the Materials Project.
https://pymatgen.org
Other
1.45k stars 842 forks source link

Structure.to method silently ignores user specified file format #1614

Closed PetMetz closed 4 years ago

PetMetz commented 4 years ago

Describe the bug (1) pymatgen.Structure.to method silently writes YAML output if the requested structure file type is not in the decision tree.

(2) Writing Protein Data Bank (.pdb) structure format evidently supported in pymatgen.io.lammps.utils, but not in Structure.to method (this is probably a separate complaint).

To Reproduce Steps to reproduce the behavior:

  1. create a Structure instance
  2. run Structure.to(fmt="nonsense", filename="more.nonsense")

Expected behavior Structure.to method admits it is unable to fulfill your request for nonsense (raise if structure format not supported).

Desktop (please complete the following information):

mkhorton commented 4 years ago

Hi @PetMetz, thanks for opening this.

  1. is not a bug (though may not be the best decision either); this is a default output.

  2. yes, this is true; many formats are missing in the .to method, including formats supported via the ASE and OpenBabel adaptors.

I think a few things might help here. First, I think keeping a list of supported outputs from Structure.to would be helpful. Secondly, I agree that raising an exception makes more sense than falling back to a default (if you want json/yaml you can ask for it). Thirdly, we should at least make sure to include all formats we support natively (e.g. the lammps format) and let the users know about the ASE/OpenBabel adaptors.

There are some other improvements to be made also; that method has a lot of duplicate code for writing to file, it'd be better to get the string representation first and write to file as a final step. Also the from_file and from_str methods are inconsistent and from_file should use from_str directly.

shyuep commented 4 years ago

I pushed a change that fixes item 1. But for item 2, I believe that PDB is actually supported if you have Openbabel installed.

PetMetz commented 4 years ago

Thanks @mkhorton and @shyuep for the quick responses.

I have installed Open Babel (or Pybel, anyway), but the file formats supported by the pymatgen.Structure.to method seem to be

which doesn't seem to cover all the methods in pymatgen.io, or include .pdb.

This is getting off-topic from the title of this thread though, so I suppose we should probably wrap this up.

-Peter

shyuep commented 4 years ago

Ok. Yeah, I usually don't think of PDB files as a Structure type. Feel free to add the function at a time of your convenience.