brownbp1/addmedchem_element_liberty #4
Conversation
…pseudo-reactions. Also added some usage info.
jeffmendenhall
left a comment
jeffmendenhall
left a comment
There was a problem hiding this comment.
see comments; think some type changes would be helpful for clarity and performance
| // this will cause issues so it's banned | ||
| if | ||
| ( | ||
| ( m_TargetMoleculeLinkElementType.empty() && m_EnableDummyAtom ) || |
There was a problem hiding this comment.
Why not just have m_EnableDummyAtom=!m_TargetMoleculeLinkElementType.empty(); in the rest of the logic. If I give a list of linked element types, it seems I must want them, no?
| std::string m_MedChemFilename; | ||
|
|
||
| //! alternate element types to Undefined to control directionality | ||
| std::string m_MedChemFragmentLinkElementType; |
There was a problem hiding this comment.
Why aren't these chemistry::ElementType ? or lists of them if that's more appropriate?
Not that performance is an issue here, but it does lots of needless lookups later where it would seem better to just have an element type instance unless I'm missing something
| ( | ||
| !m_TargetMoleculeLinkElementType.empty() && | ||
| m_EnableDummyAtom && | ||
| m_MutableElements.Find( GetElementTypes().ElementTypeLookup( m_TargetMoleculeLinkElementType)) >= m_MutableElements.GetSize() |
There was a problem hiding this comment.
Once you make those variables element types or lists thereof, can remove the elementtypelookup here
| const ElementType medchem_fragment_link_element | ||
| ( | ||
| m_MedChemFragmentLinkElementType.empty() ? GetElementTypes().e_Undefined : GetElementTypes().ElementTypeLookup( m_MedChemFragmentLinkElementType) | ||
| ); | ||
| const ElementType target_molecule_link_element | ||
| ( | ||
| m_TargetMoleculeLinkElementType.empty() ? GetElementTypes().e_Undefined : GetElementTypes().ElementTypeLookup( m_TargetMoleculeLinkElementType) | ||
| ); | ||
|
|
There was a problem hiding this comment.
This logic would also be unnecessary if they are just element types or lists thereof, whichever is appropriate
Minor PR:
The goal of this branch was to modify the AddMedChem mutate in the following ways:
(1) To allow users to specify element types for pseudo-reaction linking. Previously only unknown element types (X) were allowed. This created problems in the BCL-Rosetta integration. This is the easiest way to resolve, plus it adds a layer of convenience.
(2) Added some usage info in the help options.