From 0e67f39405ebe9d94efbea1c77d40e05caa85adc Mon Sep 17 00:00:00 2001 From: Tim Dudgeon Date: Wed, 10 Jun 2026 13:36:07 +0100 Subject: [PATCH] Simplify CIF helper functions in pdb_deposition.py - Remove unused functions: append_data_to_values and prune_loop - Remove dict_to_tags_values (trivial); inline at call site with list(d.keys())/list(d.values()) - Inline add_row_if_not_excluded into collect_entity_values - Merge delete_pair_item + delete_loop_item into delete_cif_items (keyword args select item type) - Merge append_loop_items + combine_diffrn_loops into build_loop (None-safe, handles both use cases) Closes #85 Co-Authored-By: Claude Sonnet 4.6 --- src/pdbdepo/pdb_deposition.py | 181 ++++++++-------------------------- 1 file changed, 43 insertions(+), 138 deletions(-) diff --git a/src/pdbdepo/pdb_deposition.py b/src/pdbdepo/pdb_deposition.py index 8fcc2a5..d36067f 100644 --- a/src/pdbdepo/pdb_deposition.py +++ b/src/pdbdepo/pdb_deposition.py @@ -305,16 +305,16 @@ def process_input( structure_cif_doc.write_file(str(xtal_out_path / 'original.cif')) # find and delete the _exptl loop as mmcifgen handles this - delete_pair_item(structure_cif_doc, '_exptl') + delete_cif_items(structure_cif_doc, '_exptl', pairs=True) merge_entity(xtal_name, structure_cif_block0, mmcif_gen_entity_tags, mmcif_gen_entity_values) # do some cleaning - delete_pair_item(structure_cif_doc, '_struct') - delete_pair_item(structure_cif_doc, '_struct_keywords') - delete_pair_item(structure_cif_doc, '_pdbx_database_status') - delete_loop_item(structure_cif_doc, '_exptl') - delete_loop_item(structure_cif_doc, '_reflns') + delete_cif_items(structure_cif_doc, '_struct', pairs=True) + delete_cif_items(structure_cif_doc, '_struct_keywords', pairs=True) + delete_cif_items(structure_cif_doc, '_pdbx_database_status', pairs=True) + delete_cif_items(structure_cif_doc, '_exptl', loops=True) + delete_cif_items(structure_cif_doc, '_reflns', loops=True) refine_item = find_loop_item(structure_cif_block0, '_refine') if refine_item: @@ -322,7 +322,7 @@ def process_input( model_refine_tags = list(refine_item.loop.tags) model_refine_values = list(refine_item.loop.values) refine_item.erase() - append_loop_items( + build_loop( structure_cif_block0, model_refine_tags, model_refine_values, @@ -333,11 +333,10 @@ def process_input( # might be pairs if generated by buster d = read_pairs(structure_cif_block0, '_refine', True) if d: - model_refine_tags, model_refine_values = dict_to_tags_values(d) - append_loop_items( + build_loop( structure_cif_block0, - model_refine_tags, - model_refine_values, + list(d.keys()), + list(d.values()), mmcifgen_refine_tags, mmcifgen_refine_values, ) @@ -432,8 +431,18 @@ def process_input( collection_info_diffrn_item = item continue structure_cif_block0.add_item(item) - combine_diffrn_loops( - collection_info_diffrn_item, mmcifgen_diffrn_tags, mmcifgen_diffrn_values, structure_cif_block0 + collection_diffrn_tags = ( + list(collection_info_diffrn_item.loop.tags) if collection_info_diffrn_item else None + ) + collection_diffrn_values = ( + list(collection_info_diffrn_item.loop.values) if collection_info_diffrn_item else None + ) + build_loop( + structure_cif_block0, + collection_diffrn_tags, + collection_diffrn_values, + mmcifgen_diffrn_tags, + mmcifgen_diffrn_values, ) # add in the _software section @@ -600,20 +609,6 @@ def scrape_aimless_version(aimless_p): return None -def dict_to_tags_values(d: dict): - """ - Convert a dict to lists of tags and values suitable for a loop - :param d: - :return: - """ - tags = [] - values = [] - for t, v in d.items(): - tags.append(t) - values.append(v) - return tags, values - - def read_mmcifgen_entity_data_and_erase(mmcifgen_block): mmcifgen_entity_item = find_loop_item(mmcifgen_block, '_entity') @@ -676,38 +671,26 @@ def collect_entity_values(tags, values, exclude_pairs=[]): d = None for i, value in enumerate(values): if i % len(tags) == 0: - if d: - add_row_if_not_excluded(d, rows, exclude_pairs) + if d and not any(d.get(k) == v for k, v in exclude_pairs): + rows.append(d) d = {} tag = tags[i % len(tags)] d[tag] = value - add_row_if_not_excluded(d, rows, exclude_pairs) + if d and not any(d.get(k) == v for k, v in exclude_pairs): + rows.append(d) return rows -def add_row_if_not_excluded(d, rows, exclude_pairs): - is_excluded = False - for exclude in exclude_pairs: - if d.get(exclude[0]) == exclude[1]: - is_excluded = True - if not is_excluded: - rows.append(d) - - -def delete_pair_item(doc, prefix): +def delete_cif_items(doc, prefix, *, pairs: bool = False, loops: bool = False): for block in doc: - for item in block: - if item.pair is not None: - if item.pair[0].startswith(prefix + '.'): + if pairs: + for item in block: + if item.pair is not None and item.pair[0].startswith(prefix + '.'): item.erase() - - -def delete_loop_item(doc, prefix): - for block in doc: - item = find_loop_item(block, prefix) - if item: - # print('erasing', item) - item.erase() + if loops: + loop_item = find_loop_item(block, prefix) + if loop_item: + loop_item.erase() def find_loop_item(block, prefix): @@ -737,94 +720,16 @@ def read_pairs(block, prefix, erase: bool): return d -def append_loop_items(block_to_add_to: cif.Block, tags1, values1, tags2, values2): - """Simplistic approach that merges two 1-row loops, add the data from item2 into item1 +def build_loop(block: cif.Block, tags1, values1, tags2, values2): + """Create a single-row loop in block by concatenating two tag/value lists. - :param block_to_add_to: the block the loop needs to be added to - :param tags1: the first tags to append - :param values1: the first values to append - :param tags1: the second tags to append - :param values1: the second values to append - :return: + # gemmi does not allow appending to an existing loop's tags/values in place + Either argument pair may be None (treated as empty). """ - - # its seems that you can't just append to the existing loop's tags and value, you have to create a new loop - combined_tags = [] - combined_values = [] - combined_tags.extend(tags1) - combined_tags.extend(tags2) - combined_values.extend(values1) - combined_values.extend(values2) - - loop = block_to_add_to.init_loop('', combined_tags) - - loop.add_row(combined_values) - - -def append_data_to_values(tags1, values1, tags2, values2, ordinal_col): - """ - Append the data is tags/values2 to that in tags/values1, ensuring the order is correct - If no value is found then ? is written - :param tags1: - :param values1: - :param tags2: - :param values2: - :return: A list of lists of the values - """ - - if ordinal_col: - ordinal_idx = tags1.index(ordinal_col) - else: - ordinal_idx = None - - combined_values = [] - combined_values.append(values1) - n = len(tags2) - dicts = [] - d = None - ordinal_count = len(combined_values) - for i, val in enumerate(values2): - ordinal_count += 1 - if i % n == 0: - d = {} - dicts.append(d) - if i % n == ordinal_idx: - d[tags2[i % n]] = str(ordinal_count) - else: - d[tags2[i % n]] = val - - for d in dicts: - l = [] - combined_values.append(l) - for tag1 in tags1: - if tag1 in d: - l.append(d[tag1]) - else: - l.append('?') - - return tags1, combined_values - - -def combine_diffrn_loops(collection_info_item: cif.Item, tags_list: list, values_list: list, into: cif.Block): - tags = [] - if collection_info_item: - tags.extend(collection_info_item.loop.tags) - if tags_list: - tags.extend(tags_list) - values = [] - if collection_info_item: - values.extend(collection_info_item.loop.values) - if values_list: - values.extend(values_list) - if tags: - new_loop = into.init_loop('', tags) - new_loop.add_row(values) - - -def prune_loop(loop, retain): - for tag in loop.tags: - if not tag.startswith(retain): - loop.remove_column(tag) + combined_tags = list(tags1 or []) + list(tags2 or []) + combined_values = list(values1 or []) + list(values2 or []) + if combined_tags: + block.init_loop('', combined_tags).add_row(combined_values) def validate_structure_cif_doc(doc: cif.Document, xtal_name: str = '') -> list[str]: @@ -851,7 +756,7 @@ def validate_structure_cif_doc(doc: cif.Document, xtal_name: str = '') -> list[s if not found: issues.append(prefix + f'required pair category {cat} not found') - # 2. Duplicate tags in any loop (catches naive concatenation bugs in append_loop_items) + # 2. Duplicate tags in any loop (catches naive concatenation bugs in build_loop) for item in block: if item.loop is not None: tags = item.loop.tags