Add support for BinaryCIF by sbittrich · Pull Request #839 · biojava/biojava

sbittrich · 2019-05-02T20:46:41Z

What is BinaryCIF

BinaryCIF is a compact representation of CIF files
similar encoding strategies are used as in MMTF - however the format is fully compatible with conventional, text-based CIF files
therefore, we get all the advantages and established handling of CIF files as well as the performance of MMTF
ciftools-java provides the BinaryCIF codec, all model classes for the content of CIF files, and capabilities for the parsing of text-based CIF files
content from binary and text files is stored in the same data structure (a CifFile) - thus, it does not matter where the data came from: binary and text-based CIF data is handled the same way

What changed

I tried to introduce no change to existing classes and behavior, the implementation rather coexists with the established and tested parsing of CIF files
handling of BinaryCIF and CIF files is delegated to ciftools, this library provides a representation which can be used to populate Structure instances with data
parsing behavior was taken from SimpleMMcifConsumer - no information should be missing
the majority of tests we duplicated for now to work on the new implementation (both on binary and text-based CIF files)

Performance

Some basic performance tests.

The whole PDB archive (in parallel)

parsing 151079 BinaryCIF files takes 918 s
parsing 151079 (text-based) CIF files takes 2502 s (a new experimental implementation for text-based CIF parsing which comes basically for free by wrapping ciftools)

5000 PDB structures (in parallel)

parsing 5000 BinaryCIF files takes 35 s
parsing 5000 (text-based) CIF files with the new CIF parser takes 132 s
parsing 5000 (text-based) CIF files with the current BioJava CIF parser takes 310 s
parsing 5000 MMTF files (fetched from the web) with the BioJava MMTF parser the takes 17 s
in summary, parsing for conventional CIF files is substantially faster
performance should match MMTF when only atom coordinates are considered, BinaryCIF however provides all content from a file

Side notes

SimpleMMcifConsumer uses SimpleDateFormat for date parsing which caused a number of parsing exceptions when executed in parallel - my implementation uses Java 8's LocalDate for parsing, while still reporting Date objects compatible with the data structure

Down the road

integrate into StructureIO/AtomCache
omit/merge duplicated code if stable (SimpleMMcifConsumer, mmCIF model beans)
the new code is a good chance to revisit missing categories in the SimpleMMcifConsumer (currently not parsed content) and refine the writing in FileConvert

- solves Exceptions in binary parsing + speed-up (?) - Exception were parsing exceptions from the prominent Date class when executing in parallel. Not possible to reproduce, but omitted by changing to robust, thread-safe number parsing class.

lafita

Many thanks for this @JonStargaryen ! The code looks good and clean to me, I just had a couple of minor comments. I will leave it open for others to have a more thorough review, though.

- isolates consumer and converter functions - omits highly specific performance test

josemduarte

This looks fantastic, thank you so much! It adds a much more versatile and performant CIF parser compare to the current one, plus support for binary CIF. In my opinion we should eventually replace the current parser by this one, but that would be quite a dramatic change that breaks some API compatibility, so it needs to go with release 6.0. In the meantime we could release 5.3.0 that adds this parser as a second option. One thing to consider for a separate PR is whether to add full AtomCache functionality for binary cif.

I have one comment/doubt about the hard-coded atom_site field names, could you clarify that?

josemduarte · 2019-05-11T02:04:44Z

+
+        AtomSiteCollector() {
+            // TODO this doesn't really make the case for the builder ;)
+            this.groupPDB = new StrColumnBuilder<>("atom_site", "group_PDB", null);


I thought these hard-coded strings shouldn't be needed. They should come from metadata available via the CIFTools library. Or am I missing something?

Thanks for pointing this out. 196e0c5 replaces the hard-coded category and column names with the corresponding builder functions.

heuermh · 2019-05-11T02:18:57Z

Curious, is there a reason why the code delegated to here is in a separate repository instead of a separate module in biojava?

sbittrich · 2019-05-13T17:07:20Z

Curious, is there a reason why the code delegated to here is in a separate repository instead of a separate module in biojava?

The ciftools code is not BioJava-specific. Rather it provides generic functionality for the handling of CIF files which can also be used by other projects. Therefore, I opted to create a separate project.

heuermh · 2019-05-13T18:42:59Z

The ciftools code is not BioJava-specific.

Well, the scope of BioJava is what is in BioJava. There wasn't any support for 3D structures in BioJava until there was. :)

I would much rather have ciftools-java exist within BioJava than add a new dependency on an external single-developer project. That will help make deprecation/refactoring/removal of the current CIF implementation in BioJava easier.

sbittrich · 2019-05-13T22:01:49Z

I would much rather have ciftools-java exist within BioJava than add a new dependency on an external single-developer project. That will help make deprecation/refactoring/removal of the current CIF implementation in BioJava easier.

That sure is a valid concern. The basic (binary) CIF handling is realized by dedicated, language-specific projects to be able to publish/promote the format as well as codec implementations. A project is more visible than a module somewhat hidden in BioJava. The same was done for MMTF.

Also, don't worry about maintenance as ciftools-java is a RCSB project. Jose has admin rights to the repo and we're literally one desk apart for the case that refactoring becomes an issue.

josemduarte

Thanks for implementing the changes!

josemduarte · 2019-05-17T00:04:34Z

@heuermh just want to reiterate that outsourcing to an external library is the same as was done for the mmtf format. Additionally, in this case, the new CIFtools library is more general than just parsing BinaryCIF. It provides all the tooling needed for dealing with the CIF format and CIF schemas. So it does a lot more that what we would want to have in BioJava.

josemduarte · 2019-05-29T23:53:20Z

I'll merge this by the end of this week if there are no other concerns.

Sebastian Bittrich added 20 commits April 19, 2019 10:38

implements a unified CIF and BinaryCIF parser

3ab6042

stub for CifFileSupplier

e35eb9b

reading and writing of Cif/Bcif files, integration

9f60663

writing of Cif files for AltLocs

e4f654a

updates CifFileSupplier to infer types

f09a417

full model, type-safe builder integration, fetch URL

6ddc053

moves some tests to integration

6d8e3e7

removes modifications to AtomCache again

2c90336

updates to ciftools from maven

4824817

stub for to test date format parsing

9f05a76

removes generic getColumn calls

f26563d

adds AltLoc NullPointerException test

7c03622

bug fix where three-letter-code was not assigned for altloc

818354e

removes sout

eeadc2f

adds test results, small cleanup in CifFileConsumerImpl

4644ce1

removes unnecessary changes

4b96a56

Merge remote-tracking branch 'upstream/master' into bcif

6c03a23

Merge remote-tracking branch 'upstream/master' into bcif

f345999

updates MMTF performance

f410aa5

lafita added the new feature New method or data structure label May 7, 2019

lafita requested review from josemduarte and pwrose May 7, 2019 14:57

lafita reviewed May 7, 2019

View reviewed changes

Comment thread ...rationtest/src/test/java/org/biojava/nbio/structure/test/io/cif/CifFileSupplierImplTest.java Resolved

Comment thread ...rationtest/src/test/java/org/biojava/nbio/structure/test/io/cif/CifFileConsumerImplTest.java Resolved

Sebastian Bittrich added 5 commits May 7, 2019 09:52

removes star imports

0b59a10

should remove formatting issues for files which did not change

78ed625

renames tests in integration module

1755a4c

cleanup and updates to ciftools 0.4.1

6bba4e1

- isolates consumer and converter functions - omits highly specific performance test

updates version tag in files

0e2acb6

josemduarte requested changes May 11, 2019

View reviewed changes

replaces hard-coded category and column names during cif building

196e0c5

josemduarte approved these changes May 17, 2019

View reviewed changes

lafita removed the request for review from pwrose May 17, 2019 08:27

josemduarte merged commit 5d79d90 into biojava:master Jun 3, 2019

This was referenced Sep 2, 2019

Add support for parsing secondary structure in mmCIFF #333

Open

Advance notice: next release 5.3.0 #848

Closed

Test performance of various ways of loading PDB data from local files. #590

Closed

Navigation Menu

Repository navigation

Add support for BinaryCIF - #839

Add support for BinaryCIF#839
josemduarte merged 26 commits into
biojava:masterfrom
sbittrich:bcif

sbittrich commented May 2, 2019 •

edited

Loading

Uh oh!

lafita left a comment

Uh oh!

Uh oh!

Uh oh!

josemduarte left a comment

Uh oh!

josemduarte May 11, 2019

Uh oh!

sbittrich May 13, 2019

Uh oh!

heuermh commented May 11, 2019

Uh oh!

sbittrich commented May 13, 2019

Uh oh!

heuermh commented May 13, 2019 •

edited

Loading

Uh oh!

sbittrich commented May 13, 2019

Uh oh!

josemduarte left a comment

Uh oh!

josemduarte commented May 17, 2019

Uh oh!

josemduarte commented May 29, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sbittrich commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is BinaryCIF

What changed

Performance

The whole PDB archive (in parallel)

5000 PDB structures (in parallel)

Side notes

Down the road

Uh oh!

lafita left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

josemduarte May 11, 2019

Choose a reason for hiding this comment

Uh oh!

sbittrich May 13, 2019

Choose a reason for hiding this comment

Uh oh!

heuermh commented May 11, 2019

Uh oh!

sbittrich commented May 13, 2019

Uh oh!

heuermh commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbittrich commented May 13, 2019

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

josemduarte commented May 17, 2019

Uh oh!

josemduarte commented May 29, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbittrich commented May 2, 2019 •

edited

Loading

heuermh commented May 13, 2019 •

edited

Loading