Improve BinaryCIF float chain encoding#52
Conversation
hriday1136
left a comment
There was a problem hiding this comment.
Added suggestions to be considered. No major changes at all.
| return 10 ** mantissaDigits | ||
|
|
||
| # Try each FixedPoint integer chain and keep the smallest byte output | ||
| def __encodeBestFixedPointChain(self, colDataList, factor): |
There was a problem hiding this comment.
might want to see whether it is better to check all four encoding chains for each column or hardcoding one specific chain for all columns. Can be checked in terms of size and runtime tradeoff.
There was a problem hiding this comment.
The four-chain search applies only to non-hardcoded float columns with four or fewer decimal places, so although it covers many columns, those columns contain a relatively small share of the total values.
There was a problem hiding this comment.
Pull request overview
This PR improves BinaryCIF serialization for float-typed columns by introducing FixedPoint encoding with integer compression chains, using column-aware hints for high-volume structural attributes (e.g., coordinates, anisotropic U), and a general “choose smallest” chain selection for other float columns, while preserving existing mask semantics.
Changes:
- Pass
(catName, atName)through the internal encoding pipeline so float encoders can apply item-specific policies. - Add
FixedPointencoder support and propagate the working data type through chained encoders so downstreamByteArraymetadata matches the transformed integer data. - Implement float-chain selection logic (special-case hints + general factor detection + best-of-four integer chains), with ByteArray fallback paths for unsafe data.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Updated comments for clarity and adjusted descriptions of encoding options.
|
So I ran the tox tests and picked up on some style issues: mmcif/io/BinaryCifWriter.py:208:5: E303 too many blank lines (2) lint_pylint-py39: commands[1]> pylint --disable=R,C --reports=n --rcfile=/private/tmp/py-mmcif/pylintrc mmcif/ For pylint issue - it is a false report... I would add at the end of the line " # pylint: disable E1136" |
| def encodeWithMask(self, colDataList, colMaskList, encodingType, catName=None, atName=None): | ||
| """Encode the data using the input mask and encoding type returning encoded data and encoding instructions. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Docstring should be updated for new arguments
epeisach
left a comment
There was a problem hiding this comment.
I have provided comments inline with code... The largest change involves your hints - should use an Enum, and then two dictionaries - enabled, and configuration.
|
|
||
| # True: enable the new FixedPoint-based float encoding logic and all enabled | ||
| # specialized paths below. | ||
| # False: bypass every float optimization below and encode all floats directly |
There was a problem hiding this comment.
Why are these switches accessible from outside the class? They should be private. If you want to add an interface to switch on or off internal flags -- create it. Abstraction barriers matter.
There was a problem hiding this comment.
I agree with Ezra here—they should at minimum be private, i.e.,:
_USE_COORDINATE_CHAIN = True
_USE_ANISOTROP_U_CHAIN = True
...There was a problem hiding this comment.
Actually, Ezra and I talked about this and came up with a decision on what to do here. I'll share in my parent review comment.
|
|
||
| if encType == "ByteArray": | ||
| colDataList, encDict = self.byteArrayEncoderTyped(colDataList, dataType) | ||
| colDataList, encDict = self.byteArrayEncoderTyped(colDataList, currentDataType) |
There was a problem hiding this comment.
Add a comment explaining why currentDataType is changed from float to integer for the byte packing code...
|
|
||
| def __encodeColumnData(self, colDataList, dataType): | ||
| # Accept category/item names so encoder selection can use column-specific hints | ||
| def __encodeColumnData(self, colDataList, dataType, catName=None, atName=None): |
There was a problem hiding this comment.
No need to have a default ofr catName or atName. __encodeColumnData is invoked only one place in the code.
There was a problem hiding this comment.
Although it's only used once in the code, I think having non-positional parameters with defaults is better practice, since it avoids having to worry about the order in which arguments are passed later.
| srcType = colTypedDataList.dtype or ("float_64" if self.__useFloat64 else "float_32") | ||
| encodedColDataList = [self.__roundLikeMolStar(float(v) * factor) for v in colTypedDataList.data] | ||
|
|
||
| if not self.__fitsInt32(encodedColDataList): |
There was a problem hiding this comment.
It is not clear what to do here... Exception is the best right now - as I have no idea how to rollback and encode as float_32/float_64....
| if catName is None or atName is None: | ||
| return "" | ||
|
|
||
| cat = str(catName) |
There was a problem hiding this comment.
Do you envision that catName will not be a string?
piehld
left a comment
There was a problem hiding this comment.
Thank you @yusufm99, this is fantastic work!
I've left a couple comments and questions for you throughout the code, as well as resolved some of Ezra's comments after having discussed some ideas with him, so when you review our comments please do so on GitHub (as that will hide any comments that have already been marked as resolved).
The main change @epeisach and I discussed is to move all the "switches" and other pre-defined factors to a separate configuration file config.py, and merge the different data item lists together such that you don't need so many "if/else" checks to use them. To give you an idea of what it should look like, I'll send you a draft of the file by email. You would then import the config class into the BinaryCifWriter.py file and access the different config options from the config object.
Another change that Ezra and I thought would be worth including is to encode all floats with ≤ 2 rows as strings (i.e., if there are only two rows in a particular float column, encode it as a string rather than a float). This should be controlled by another configuration setting (e.g., STRING_FALLBACK_MAX_FLOAT_ROWS). However, before you proceed with implementing that, I'm wondering if you think it'd make more sense to handle this in @hriday1136's code? Would that be a better place to determine if the type should be "string" or "float" based on the number of rows?
| # | ||
| # Float encoding updates: | ||
| # - FixedPoint support with IntegerPacking, RunLength, and Delta chains. | ||
| # - Column-specific chains for known high-volume float attributes. | ||
| # - Automatic selection of the smallest general FixedPoint chain. | ||
| # - Optional StringArray fallback for high-precision floats (disabled by default). |
There was a problem hiding this comment.
Let's keep the top-level "Updates" comment there, and add yours as a specific update (dating ahead to 10-Jul-2026)
| # | |
| # Float encoding updates: | |
| # - FixedPoint support with IntegerPacking, RunLength, and Delta chains. | |
| # - Column-specific chains for known high-volume float attributes. | |
| # - Automatic selection of the smallest general FixedPoint chain. | |
| # - Optional StringArray fallback for high-precision floats (disabled by default). | |
| # Updates: | |
| # 10-Jul-2026 ym Float encoding updates: | |
| # - FixedPoint support with IntegerPacking, RunLength, and Delta chains. | |
| # - Column-specific chains for known high-volume float attributes. | |
| # - Automatic selection of the smallest general FixedPoint chain. | |
| # - Optional StringArray fallback for high-precision floats (disabled by default). |
|
|
||
| def __encodeColumnData(self, colDataList, dataType): | ||
| # Accept category/item names so encoder selection can use column-specific hints | ||
| def __encodeColumnData(self, colDataList, dataType, catName=None, atName=None): |
There was a problem hiding this comment.
Although it's only used once in the code, I think having non-positional parameters with defaults is better practice, since it avoids having to worry about the order in which arguments are passed later.
| # specialized paths below. | ||
| # False: bypass every float optimization below and encode all floats directly | ||
| # with the original float ByteArray behavior. | ||
| USE_FIXED_POINT_FLOAT_ENCODING = True |
There was a problem hiding this comment.
I think by default we always want this (unless useStringTypes=True), right @epeisach? So, I would suggest this switch be removed.
|
|
||
| # True: enable the new FixedPoint-based float encoding logic and all enabled | ||
| # specialized paths below. | ||
| # False: bypass every float optimization below and encode all floats directly |
There was a problem hiding this comment.
I agree with Ezra here—they should at minimum be private, i.e.,:
_USE_COORDINATE_CHAIN = True
_USE_ANISOTROP_U_CHAIN = True
...|
|
||
| # True: enable the new FixedPoint-based float encoding logic and all enabled | ||
| # specialized paths below. | ||
| # False: bypass every float optimization below and encode all floats directly |
There was a problem hiding this comment.
Actually, Ezra and I talked about this and came up with a decision on what to do here. I'll share in my parent review comment.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Leave only one blank line between adjacent methods.
Optimize BinaryCIF float encoding with FixedPoint chain selection
Summary
This PR improves BinaryCIF serialization of floating-point columns in
mmcif/io/BinaryCifWriter.py.The production writer previously encoded float columns directly as
ByteArray. This change adds BinaryCIFFixedPointencoding followed by integer compression chains. It uses specific encoding rules for known high-volume structural fields and automatically selects the smallest supported chain for other float columns.The final writer was tested on these 10 structures:
2BVK,4HHB,12GB,4BTS, and3J3Q9A01,9A25, and9A8KAF_AFA0A017SEY2F1MA_MABAKCEPC0001File-size comparison
The BCIF file sizes for all 10 structures were added together for each writer:
The size reduction was calculated as:
Serialization-time comparison
Each structure was serialized five times with each writer. For each structure, the median time from the five measured runs was selected. The median times for all 10 structures were then added together:
Production writer: 45.756497 seconds
Final writer: 61.785365 seconds
Overall time increase: 35.031%
The time increase was calculated as:
The change is intentionally limited to one production file. It does not change the decoder, public writer API, dictionary loading, BinaryCIF version metadata, or package dependencies.
Motivation
BinaryCIF is column-oriented and supports chaining encoders such as
FixedPoint,Delta,RunLength,IntegerPacking, andByteArray. The existing py-mmcif writer encoded float columns directly asByteArray. This is valid, but it does not take advantage of the additional compression available for coordinates and other structured numeric columns.This implementation follows the BinaryCIF encoding approach used in Mol* and supported by the
ciftools-javalibrary:FixedPoint;ByteArrayencoding whenFixedPointcannot be used safely.The goal is to produce substantially smaller BCIF files while preserving the intended numeric precision and ensuring that the files can still be correctly read by other BinaryCIF implementations, including Mol* and
ciftools-java.Scope
rcsb/py-mmcif:dev-bcif-stagingyusufm99/py-mmcif:dev-bcif-chain-encoding096e052ba56353c6b867cacb175824dc8a7174f4mmcif/io/BinaryCifWriter.pyNo benchmark scripts, generated BCIF files, logs, backup writers, or local paths are included in the commit.
What changed and why
This section documents both the implementation and the reasoning behind each decision.
The implementation separates two kinds of behavior:
_ihm_sphere_obj_site.object_radiusrule, the explicit RunLength hints, overflow fallback, feature switches, and the decision to keep StringArray fallback disabled.The internal evidence paths below refer to the evidence package generated for repository commit:
1. Pass column identity into the float encoder
Problem
The production float path received the values and mask for a column, but it did not receive the category or attribute name.
Without that context, the writer could not apply a deliberate precision or encoding policy to known structural fields. Coordinates, anisotropic displacement values, sphere radii, repeated B-factor fields, and unrelated general-purpose float columns all reached the same direct float
ByteArraypath.External basis
Mol* supports encoding-strategy hints that identify a field by category and column name and optionally specify its float precision:
EncodingStrategyHintChange
The internal call path now forwards
catNameandatName:The float encoder normalizes those values into an item name of the form:
This allows it to select a specialized policy when one exists and the general float classifier otherwise.
The public interface remains unchanged:
No constructor argument or public method signature was added.
Validation
The final writer passed:
Result and decision
Column context is retained as an internal implementation detail because it enables precision and chain selection without changing the writer API or changing how dictionary types are assigned.
Internal evidence:
2. Add a typed FixedPoint encoder and track type changes through the chain
Problem
The production writer encoded dictionary-typed float columns directly as float
ByteArraydata.That representation is valid, but it does not allow the writer to exploit the lower dynamic range and repeated patterns that become available after decimal float values are converted into scaled integers.
A second issue was type propagation. Once
FixedPointtransforms floats into integers, all later encoders must treat the working array as integer data. Continuing to pass the original float type toByteArraywould produce incorrect encoding metadata and packing behavior.External basis
BinaryCIF defines
FixedPointas a float-to-Int32transformation containing:kind;factor;srcType.See:
FixedPointmetadata definitionMol* performs the conversion using
Math.round:The Mol* source also contains a TODO requesting a better overflow check before values are committed to an
Int32Array.Change
A typed
fixedPointEncoderTyped()implementation now:float_32orfloat_64source data;Math.round-compatible behavior;TypedArraywith data typeinteger_32;FixedPointmetadata.The rounding helper uses:
This reproduces the relevant JavaScript
Math.roundbehavior, including its treatment of negative half values.The generic chain executor now tracks the working data type. After
FixedPoint, laterDelta,RunLength,IntegerPacking, andByteArraystages operate on integer data rather than on the original float type.Safety behavior
FixedPoint is not used when:
NaN;int32range;Those cases use the established numeric float
ByteArraypath.Validation
The focused fallback test verified five direct cases:
The 37-structure round-trip comparison then checked 143,178,047 values and reported:
All 37 generated files also passed the independent Java encoding verifier.
Result and decision
Typed FixedPoint encoding is enabled by default. Explicit finite-value and signed-
int32checks were retained because they make failure behavior deterministic and address a safety concern that the Mol* classifier itself identifies.Internal evidence:
3. Detect a safe FixedPoint factor for general float columns
Problem
A fixed factor cannot be safely applied to every float column.
Some columns contain integer-valued floats, some require one to four decimal places, and some require more precision than the FixedPoint policy supports. The writer therefore needed a bounded rule for determining when a general float column can be converted to integers without blindly applying a factor.
External basis
The Mol* float classifier:
1e-6;ByteArraywhen a safe conversion cannot be established.See:
Change
For general float columns, the writer searches these factors in ascending order:
For every value, it tests whether the scaled value lies within
1.0e-6of an integer.The greatest number of decimal places required by any value in the column determines the factor:
1101001,00010,000The detector returns no factor when:
A separate signed-
int32check is then performed on the fully scaled column before the selected encoding is accepted.Known coordinate items are intentionally different: they retain the explicit factor-
1,000coordinate policy rather than deriving their factor from the individual values in each file.Validation
The final round-trip and interoperability tests validate the resulting factors across the complete 37-structure corpus.
The final evidence package does not contain a standalone unit-test table exercising factor
1,10,100,1,000, and10,000individually. The factor order, tolerance, and bounded four-decimal behavior are directly visible in the final source, while the integration evidence verifies the resulting output across real PDB, IHM, AlphaFold, and ModelCIF data.Result and decision
Automatic factor detection is used for general float columns and for the repeated-field hints whose precision is not fixed in advance.
The search remains bounded at four decimal places to match the adopted Mol* classification model and to prevent uncontrolled integer growth.
Internal evidence:
4. Compare four integer chains for general FixedPoint columns
Problem
After a float column is converted to integers, no single integer chain is best for every data pattern.
For example:
Always forcing one chain would improve some columns while making others larger.
External basis
Mol* considers the same four integer strategies:
IntegerPackingRunLength → IntegerPackingDelta → IntegerPackingDelta → RunLength → IntegerPackingSee:
Change
When a safe factor is available, py-mmcif evaluates these candidates in this order:
The candidate chains reuse py-mmcif’s existing Delta and RunLength encoders. These pre-existing encoders decline to transform columns with 40 or fewer values, and RunLength also declines when its value/count representation would contain more integers than the input. The threshold of 40 already exists in the current py-mmcif BinaryCifWriter script and was not introduced or independently selected by this change.
Validation
The controlled benchmark validates the complete final classifier against the production writer on ten structures under identical conditions.
Across the complete ten-structure set:
The additional classification work increased serialization time:
These numbers measure the complete final implementation. They do not isolate the contribution of the four-chain classifier from the specialized hints.
Result and decision
The four-chain comparison was retained because it provides broad automatic coverage and produced a substantial aggregate size reduction, with the serialization-time increase documented as the principal tradeoff.
Internal evidence:
5. Use factor 1,000 and a Delta-based chain for known coordinates
Problem
Coordinate columns are among the largest numeric columns in structural data. Encoding them directly as 32- or 64-bit floating-point byte arrays misses substantial compression.
Allowing each coordinate column to infer its own precision would also make the stored coordinate precision depend on the particular values present in the file rather than on one deliberate coordinate policy.
External basis
Mol* exposes a predefined factor-
1,000FixedPoint coordinate-style chain:See:
fixedPoint3Change
Known Cartesian coordinate items request:
Expanded coordinate coverage
The project expanded the coordinate hint beyond
_atom_siteto known coordinate-bearing items from:This expanded item set is a py-mmcif project decision. It should not be presented as an exact copy of the Mol* field list.
Coordinate-factor experiment
Factors
1,000,100,000, and1,000,000were evaluated during development.The higher factors:
Validation
The final factor-
1,000configuration passed:The final benchmark showed particularly large reductions for coordinate-heavy files, but those file-level results reflect the complete implementation and should not be attributed only to the coordinate rule.
Result and decision
Factor
1,000was retained because it:Development evidence:
Final evidence:
6. Add project-specific hints for anisotropic U values, IHM sphere radii, and repeated fields
The general four-chain classifier provides broad coverage, but development audits identified several high-volume fields with stable precision requirements or predictable value patterns.
These specialized decisions were developed from the project data. They are not claimed to be exact field rules copied from Mol*.
6.1 Six anisotropic-U fields
Problem
The six independent
_atom_site_anisotrop.U[i][j]values commonly require four-decimal handling rather than the three-decimal coordinate policy.Encoding them as direct floats leaves compression available, while using factor
1,000could remove a decimal place that is intentionally represented in these columns.Change
The following fields request factor
10,000:Their requested chain is:
Validation
4BTS, which contains anisotropic-U data, passed:Result and decision
Factor
10,000was retained to preserve four-decimal U-matrix handling while allowing the values to use the integer compression pipeline.Internal evidence:
6.2
_ihm_sphere_obj_site.object_radiusProblem
IHM sphere radii are numeric, often high-volume, and suitable for FixedPoint conversion, but their ordering does not necessarily make Delta useful and their values do not necessarily form long repeated runs.
Change
_ihm_sphere_obj_site.object_radiusrequests:No fixed Delta or RunLength stage is requested.
Validation
The IHM validation corpus, including
9A8Kand9A25, passed the Python and Java checks. Representative IHM output also parsed and rendered in Mol*.Result and decision
The direct FixedPoint-plus-IntegerPacking rule was retained as the explicit policy for this known IHM field.
Internal evidence:
6.3 Repeated high-volume float fields
Problem
Several large float columns frequently contain long repeated runs. Running the full four-candidate classifier on each known case adds classification work even when the expected useful pattern is already known.
Change
These four fields receive an automatically detected factor followed by a requested RunLength chain:
The requested chain is:
The factor is not hard-coded. It is determined from the actual values using the same bounded four-decimal factor detector as other general floats.
The existing RunLength guard prevents this hint from expanding the integer stream. When RunLength would produce more integers than the original array, that stage is omitted.
Validation
The final PDB and IHM corpus passed all round-trip and Java compatibility checks.
The aggregate benchmark validates the complete final configuration. It does not prove a separate percentage contribution for each of the four RunLength-hinted fields.
Result and decision
The hints were retained because they reflect observed repeated-value patterns, avoid running all four candidate chains for established high-volume cases, and retain the existing guard against harmful RunLength expansion.
Internal evidence:
7. Preserve existing mask behavior through the new float chains
Problem
Some CIF float columns contain special missing-value markers:
.means the value was not specified;?means the value is unknown.These markers are not numbers, so they cannot be passed directly through numeric encoders such as
FixedPoint,Delta,RunLength, orIntegerPacking.External basis
BinaryCIF stores missing-value information in a separate mask:
See:
Change
The existing py-mmcif mask behavior was preserved when the new float-encoding chains were added.
For example, this logical column:
is handled as two separate arrays.
The numeric data stream becomes:
The mask becomes:
The temporary
0.0values allow the numeric data to pass safely through the new compression chain.The mask keeps the original meaning at each position:
0.0with mask1is restored as.;0.0with mask2is restored as?;0.0with mask0remains a real numeric zero.When a mask is needed, it is:
unsigned_integer_8;RunLength → ByteArray;maskobject.When every value is present, no mask object is written.
Validation
The 37-structure round-trip validation reported:
The same BCIF files also passed the Java encoding verifier, and representative files loaded successfully in Mol*.
Result and decision
The mask system itself was not redesigned by this change.
The existing separate-mask behavior was kept so that missing and unknown values remain correct while the numeric value stream can safely use the new
FixedPointand integer-encoding chains.Internal evidence:
8. Keep unsafe and high-precision fallback columns numeric
Problem
Some float columns cannot safely use the bounded FixedPoint path because they:
int32after scaling;An experimental alternative converted certain high-precision values to
StringArrayMasked.Although BinaryCIF supports
StringArray, the experimental fallback encoded dictionary-defined float columns as strings. After decoding, downstream dictionary-aware code could therefore receive string values where numeric float values were expected, creating type mismatches and possible conversion or writing errors. Keeping the fallback disabled preserves the expected numeric representation.Change
The final default is:
Therefore, unsupported general float columns remain numeric and use:
The byte width continues to follow the writer’s existing
useFloat64setting.This fallback also applies to unsafe specialized paths. A coordinate, anisotropic-U, sphere-radius, or repeated-field hint does not force FixedPoint when its values are non-finite or do not fit signed
int32.Validation
The focused final test confirmed that:
NaNusesByteArray;ByteArray;ByteArray;int32overflow usesByteArray;The 37-structure round-trip audit found no raw-type mismatches, and all outputs passed the Java verifier.
Result and decision
The StringArray fallback remains in the implementation for possible future evaluation, but it is disabled in this PR.
The final writer keeps dictionary-defined float columns numeric because that is the lower-risk choice for type semantics and interoperability.
Internal evidence:
9. Retain internal feature switches used during development
Problem
The implementation needed a controlled way to isolate the general FixedPoint path and each specialized rule during development.
Without separate switches, changing one rule at a time would require repeatedly editing the encoder logic and would make comparisons more error-prone.
Change
BinaryCifEncoderscontains these internal class-level switches:TrueFalseUSE_FIXED_POINT_FLOAT_ENCODINGTrueByteArraybehavior for all float columnsUSE_COORDINATE_CHAINTrue1,000withDelta → IntegerPacking1,000but use general four-chain selectionUSE_ANISOTROP_U_CHAINTrue10,000withDelta → IntegerPackingUSE_OBJECT_RADIUS_CHAINTrue1,000withIntegerPackingUSE_RUN_LENGTH_FLOAT_HINTSTrueUSE_STRING_FLOAT_FALLBACKFalseStringArrayMaskedByteArraydataExcept for the master FixedPoint switch, disabling one specialized switch does not force that field directly to
ByteArray. It sends the field through the general FixedPoint logic instead.Validation scope
The final validation package tests the production defaults shown above.
Result and decision
The switches remain internal class attributes because they make the individual policies reviewable and allow for quick troubleshooting and experimenting.
Internal evidence:
Before-and-after summary
ByteArray1,000with requestedDelta → IntegerPacking → ByteArrayByteArray10,000with requestedDelta → IntegerPacking → ByteArray_ihm_sphere_obj_site.object_radiusByteArray1,000withIntegerPacking → ByteArrayByteArrayRunLength → IntegerPacking → ByteArrayByteArrayByteArrayByteArrayByteArrayByteArrayint32scalingByteArrayByteArrayThe final result is a deliberate size-versus-write-time tradeoff:
Controlled benchmark
Methodology
useFloat64=TrueBinaryCifWriter.serialize(), including output-file writingAggregate result
Additional aggregate statistics:
Per-structure results
2BVK4HHB12GB4BTS3J3Q9A019A259A8KAF_AFA0A017SEY2F1MA_MABAKCEPC0001Raw benchmark artifacts generated during validation:
benchmark_runs.tsv;benchmark_summary.tsv;benchmark_aggregate.tsv;benchmark_metadata.txt.Interpretation
The optimization strongly favors reduced output size over write speed:
The aggregate result should be used for the overall tradeoff. A simple arithmetic average of individual percentages can overstate the effect of small files and is not used as the primary result.