BCIF Encoding Improvements: Fixed-Point support and Auto-type detection - #56
Conversation
…tion tests alongside dictionaryApi
Updated comments for clarity and adjusted descriptions of encoding options.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…at columns as strings
…/py-mmcif into dev-bcif-autodetect-2
BinaryCIF Auto Data Type Detection Pipeline
Improve BinaryCIF float chain encoding
Updated comments for clarity on float item configuration and FixedPoint handling.
Bcif config consolidation
BCIF config consolidation
hriday1136
left a comment
There was a problem hiding this comment.
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.
|
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. Incorporate this change or not. b) I have no other concerns. |
epeisach
left a comment
There was a problem hiding this comment.
Don't forget to update mmcif/init.py and history file.
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:
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.