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

Mol2, SDF and XYZ File Parsers #418

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

entropybit
Copy link
Contributor

@entropybit entropybit commented Aug 31, 2022

The file parsers for those 3 files now essentially work.
There is some fine work missing, especially with the hybridization heuristics in mol2.
But reading small molecules, also with multiple models, should work and I have added some more files in all 3 formats for testing this.

	to model in get_structure function needed.
	MOL2File and SDFFile prepared now ...
	files with multiple models gained from conversion via
	openbabel
	sybly atom types heuristic doesn't yet work though ...
	now seems to work. Also covering all elements in sybyl_atom_type
	now, although heuristic not fully working.
	xyz and sdf files. For mol2 files it works as long as
	the atom_name represents the element and not an atom_name like "CA"
	that are actual atom_names and not elements.
	Specifically this works with amino acids where the
	atom_names are written to the atom_name column. See
	TYR.mol2 for example in test cases.
@padix-key
Copy link
Member

Thanks for the PR. Prior to review, could you resolve the conflicts with the upstream branch?

	Before the logic for only doing that test when an amino acid
	in file name did create an empty list.
	public functions. Internal functions and members have been
	made private.
        as well as meta_information dictionaries working.
        Reading of datetime from header working. But besides that
        problems with read/write of header. These seem to stem from
        the fact that programs such as openbabel to not adher to the
        Chemical table file standard ..
        Omitting this for now as one can in principle get the header
        by acessing lines[1]
	warning causes test suite to interpret this as failure..
	save_structure. Cleaned up tests, also precision in
	XYZFiles did not match between loaded and stored.
@entropybit
Copy link
Contributor Author

Only took like a week more then expected -.- ....

Copy link
Member

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Hi, here is the first part of my review. Some general remarks:

  • I think for consistency reasons the get_structure() methods should take the model parameter analogous to for example PDBFile and select the return type based on it.
  • There are some commented out lines of code that remain.
  • Sometimes, there are (multiple) empty lines, without obvious structuring reason.
  • You can add yourself to CONTRIB.rst via this PR, if you like.

src/biotite/structure/io/ctab.py Outdated Show resolved Hide resolved
src/biotite/structure/io/ctab.py Outdated Show resolved Hide resolved
src/biotite/structure/io/general.py Outdated Show resolved Hide resolved
src/biotite/structure/io/general.py Outdated Show resolved Hide resolved
src/biotite/structure/io/general.py Outdated Show resolved Hide resolved
src/biotite/structure/io/mol2/file.py Outdated Show resolved Hide resolved
src/biotite/structure/io/mol2/file.py Outdated Show resolved Hide resolved
src/biotite/structure/io/mol2/file.py Outdated Show resolved Hide resolved
src/biotite/structure/io/mol2/file.py Outdated Show resolved Hide resolved
src/biotite/structure/io/mol2/file.py Outdated Show resolved Hide resolved
	some missing functionality with this (was not working
	properly before). Also made test_xyz.py conforming with
	PEP8 codestyle.
	Made some changes also in conver to expose header get/set
	Also added tests for this.
	that it should also work with windows paths.
	in get_structure for multi model files).
	Some cleanup...
	datetime is not matching.
	Tried to negelect this warning in the test but somehow
	mark.filterwarnings has no effect.
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