Skip to content

acedrg_tables: extract insert_bond_row / insert_angle_row - #438

Open
pemsley wants to merge 1 commit into
project-gemmi:masterfrom
pemsley:gemmi-acedrg-insert-row-pr
Open

pemsley wants to merge 1 commit into
project-gemmi:masterfrom
pemsley:gemmi-acedrg-insert-row-pr

Conversation

@pemsley

@pemsley pemsley commented Aug 24, 2026

Copy link
Copy Markdown

Extract the inline per-row index-insertion logic from load_bond_tables() and load_angle_tables() into two public methods, insert_bond_row() and insert_angle_row(), and point the loaders at them.

This deduplicates the key construction and gives external code a way to populate the bond/angle index containers row by row (e.g. an on-demand backend feeding fill_restraints without bulk-loading the whole table set). The scratch key buffers are passed in by the caller and reused across rows, avoiding per-row allocations that the old inline code made.

Behaviour-preserving: gemmi drg output is byte-identical before and after - tested against a handful of monomers.

Extract the inline per-row index-insertion logic from load_bond_tables()
and load_angle_tables() into two public methods, insert_bond_row() and
insert_angle_row(), and point the loaders at them.

This deduplicates the key construction and gives external code a way to
populate the bond/angle index containers row by row (e.g. an on-demand
backend feeding fill_restraints without bulk-loading the whole table
set). The scratch key buffers are passed in by the caller and reused
across rows, avoiding per-row allocations that the old inline code made.

Behaviour-preserving: gemmi drg output is byte-identical before and
after for ALA, ASN, ATP, CQ8, GLC, NAG, PLP and STI.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@keitaroyam
keitaroyam force-pushed the gemmi-acedrg-insert-row-pr branch from 70087d5 to fcac8ad Compare September 4, 2026 04:04
@keitaroyam

Copy link
Copy Markdown
Collaborator

I see key_buf and hybr_buf are used for performance reasons. Have you measured how much faster this is compared to defining them locally inside the function? I ask because it forces the caller to manage temporary variables.

If defining them inside the function turns out to be significantly slower, could we move key_buf and hybr_buf to be private member variables of AcedrgTables instead?

@pemsley

pemsley commented Sep 15, 2026

Copy link
Copy Markdown
Author

Thank you for your feedback - I will get back to you.

@pemsley

pemsley commented Sep 16, 2026 •

Copy link
Copy Markdown
Author

Yes, for performance reasons.

Trial-No:   -pre-bufs      +pre-bufs   delta
 1           10.551           9.620    0.931
 2           10.260           9.504    0.756
 3           10.597           9.629    0.968
 4           10.438           9.632    0.807
 5           10.521           9.735    0.786
 6           10.464           9.769    0.695
 7           10.283           9.768    0.516
 8           10.618           9.699    0.919

My understanding is that you don't much like key_buf and hybr_buf because are an implemenentational detail not appropriate for a public signature.

I will rework the commit so that they are private members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants