SKETCH-2782: get_residue_number_for_new_monomer now uses one residue number per monomer, even for nucleic acids - #372
Conversation
…number per monomer, even for nucleic acids
| if (new_monomer_ap_name == | ||
| ap_model_name_for(NASugarAP::THREE_PRIME)) { | ||
| // a new sugar bound to the next phosphate. We skip a number | ||
| // since the base is typically given the number immediately | ||
| // after the sugar | ||
| res_num_offset = -2; |
There was a problem hiding this comment.
Should we be worried about this "typically"?
There was a problem hiding this comment.
Not really. This would only be an issue if the user is intentionally leaving an abasic site (i.e. a missing base), and even then, the only result of this would be that the residues aren't numbered sequentially, which shouldn't have any real consequences. If we want a guarantee that we have the ideal residue numbering, i.e. starting at one and continuing sequentially in the correct order, we'll need to renumber the entire strand after it's been built. That's probably not worth worrying about unless we know of something downstream that's requires it.
| { | ||
| using rdkit_extensions::ChainType; | ||
|
|
||
| auto monomer_type = get_monomer_type_from_chain_type(chain_type, res_name); |
There was a problem hiding this comment.
General comment but not urgent - I'm not sure whether you will always be able to determine whether something is a base/sugar/phosphate based off residue name. I'm not sure how common it will be but there are customers with modified phosphate linkers and sugars that won't adhere to a certain naming convention - you maybe need to deduce by a substructure match, or just rely completely on the attachment points used
There was a problem hiding this comment.
Yeah, agreed. We might want to record information about the monomer type when the monomer is added, since Sketcher and/or the HELM converter would presumably be in the best position to figure that out (and the Sketcher would likely have additional information about the monomer based on the tool being used). I've filed SKETCH-2796 for this.
| new_monomer_ap_name == | ||
| ap_model_name_for(NAPhosphateAP::TO_PREV_SUGAR)) { | ||
| res_num_offset = -1; | ||
| } else if (!((monomer_type == MonomerType::NA_PHOSPHATE && |
There was a problem hiding this comment.
Wouldn't these offsets be +1 and +2? Totally possible I am not understanding the MonomerType enum correctly - is this a base connecting to a sugar then a phosphate connecting to the same sugar? if the sugar is 0, I would think that the base is 1 then the phosphate is 2
There was a problem hiding this comment.
Yeah, it looks like a bunch of the numbers here are off. Good catch! I'll fix that and update the tests, both to check for the correct results and hopefully to make those types of issues more obvious. (I also temporarily tweaked monomer drawing locally so it includes the residue numbers to verify that the current numbering is wrong, since I kept getting myself massively confused trying to reason through the numbering here.)
…take would have been obvious
| // check residue numbers for sugars bound to the phosphate | ||
| auto prev_sugar_num = get_residue_number_for_new_monomer( | ||
| "R", ChainType::RNA, ap_model_name_for(NASugarAP::THREE_PRIME), | ||
| phosphate); | ||
| auto next_sugar_num = get_residue_number_for_new_monomer( | ||
| "R", ChainType::RNA, ap_model_name_for(NASugarAP::FIVE_PRIME), | ||
| phosphate); | ||
| BOOST_TEST(prev_sugar_num == 8u); | ||
| BOOST_TEST(next_sugar_num == 11u); |
There was a problem hiding this comment.
I think it would be worth adding comments here (and on the test below) that briefly explain why we should expect these specific residue numbers.
There was a problem hiding this comment.
Yeah, that seems helpful. Added.
3 participants
Description
Based on this discussion, I've modified
get_residue_number_for_new_monomerto use one residue number per monomer, even for nucleic acids.Testing Done
Added unit tests coverage for the new logic.