Skip to content

BCIF Encoding Improvements: Fixed-Point support and Auto-type detection - #56

Merged
piehld merged 34 commits into
masterfrom
dev-bcif-staging
Sep 30, 2026
Merged

piehld merged 34 commits into
masterfrom
dev-bcif-staging

Conversation

@piehld

@piehld piehld commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

This PR combines the work from @hriday1136 and @yusufm99 for improving the BCIF encoding process by the package. Specifically, their changes introduce two major improvements:

  1. Fixed-point encoding support for floats (greatly reducing the size of encoded BCIF files) - Improve BinaryCIF float chain encoding #52
  2. Automatic data type detection (eliminating the need for providing an external dictionary reference) - BinaryCIF Auto Data Type Detection Pipeline #53

Support for providing an explicit dictionary is still supported, but by default that is now turned off and the code will instead try to infer data types.

yusufm99 and others added 30 commits June 23, 2026 12:38
Updated comments for clarity and adjusted descriptions of encoding options.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
BinaryCIF Auto Data Type Detection Pipeline
Improve BinaryCIF float chain encoding
Updated comments for clarity on float item configuration and FixedPoint handling.
@piehld
piehld requested a review from hriday1136 September 8, 2026 15:03

@hriday1136 hriday1136 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Everything seems fine after the final change. Code runs perfectly fine and all the tests in the test scripts pass without any problems. The results of testing the code came out as expected. Here is a table to review:
Note: This table show the change in in size of a mmCIF file being encoded to a BCIF file.

Metric DictionaryAPI AutoDetect Difference
Median Change in Size -34.05% -75.16% 41.11%
Avg Change in Size -20.23% -56.87% 36.65%
Median Runtime (ms) 250.45 251.40 -0.95
Avg Runtime (ms) 3042.80 3142.99 -100.20

One thing to look for in the PDB structure 11HB (pdb_000011hb):
The original mmCIF file's atom cartesian coordinates contains 5 s.f. after decimal point, whereas the encoded BCIF file's cartesian coordinates contain 3 s.f.
Example:
[11HB.cif vs 11HB.bcif] mismatch atom_site Cartn_z [4579]: '21.96274' (c1) vs. 21.963 (c2)
[11HB.cif vs 11HB.bcif] mismatch atom_site Cartn_z [4580]: '21.12001' (c1) vs. 21.12 (c2)
[11HB.cif vs 11HB.bcif] mismatch atom_site Cartn_z [4581]: '16.30905' (c1) vs. 16.309 (c2)
[11HB.cif vs 11HB.bcif] mismatch atom_site Cartn_z [4582]: '14.97524' (c1) vs. 14.975 (c2)
This is the only file that has this mismatch. All other files match 100% of their data meaning the encoding is lossless.

Another thing to look at, just for curiosity:
There are 4 structure files out of the 37 whose .bcif format file is larger than its .cif format file. This is the case with the current live encoding process too which uses dictionaryAPI.
The 4 structures are: 11BJ, 2BVK, 2XKM, and 6VC1
Here is one example :-
-rw-r----- 1 hva6 hva6 125094 Jun 4 12:19 benchmarks/data/cif/11BJ.cif
-rw-r----- 1 hva6 hva6 230180 Jun 15 18:02 benchmarks/data/bcif_gz_website/11BJ.bcif

Everything else looks fine and works fine as well. I don't think there has to be any more changes.

I will now be working on fixing the issues/errors note by the Azure testing so that Azure also passes fine.

@piehld
piehld requested a review from epeisach September 16, 2026 21:35
@epeisach

Copy link
Copy Markdown
Collaborator

In reviewing:

a) I have extended your column detector tests... As I was reading your code - I thought I might have found some edge cases. What is not documented is that a hybrid set of types will be treated as a string. This makes sense. While an integer could be treated as a float - you likely would not want to. So strings is correct - but I wanted to test further.
Also - I saw the special case fast escape if the first character of a string was 0 -- and I wanted to exercise it more.

Incorporate this change or not.

diff --git a/mmcif/tests/testBcifTypeDetector.py b/mmcif/tests/testBcifTypeDetector.py
index 4570906d..cb348cde 100644
--- a/mmcif/tests/testBcifTypeDetector.py
+++ b/mmcif/tests/testBcifTypeDetector.py
@@ -34,6 +34,13 @@ class BcifTypeDetectorTests(unittest.TestCase):
             ("int", []),
             ("int", [" 1 ", " 2 ", " 3 "]),
             ("float", [" 1.5 ", " 2.5 ", " 3.5 "]),
+            ("int", ["0", "1", "2", "3"]),
+            ("float", [0.5, 1.0, 2.5, 3.5]),
+            ("float", ["0.5", "1.0", "2.5", "3.5"]),
+            ("float", ["0.0", "1.0", "2.5", "3.5"]),                        
+            ("float", [0.0]),
+            ("str", [0.5, 1, 2.5, 3.5]),  # Hybrid mix of float and decimal
+            ("str", ["A", "B", "1.0"]),
         ]
         for expected, values in cases:
             with self.subTest(values=values):

b) I have no other concerns.

@epeisach epeisach left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't forget to update mmcif/init.py and history file.

@piehld
piehld merged commit 0095920 into master Sep 30, 2026
4 of 6 checks passed
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.

4 participants