孤立原子統合のアルゴリズム変更 #28
孤立原子統合のアルゴリズム変更 #28
Conversation
src/MoleculeToFragments.cc
Outdated
| return nRings_prev != nRings_united; | ||
| } | ||
|
|
||
| fltype calc_max_angle(const Molecule &mol, int a, int b, const vector<int> & atomids_subst_b) { |
There was a problem hiding this comment.
1件ずつの処理に分割できるにも関わらず複数件を同時に処理することになっている関数は基本的にはアヤシイと思った方が良いです。
今回のケースであれば、fltype calc_angle(const Atom &a, const Atom &b, const Atom &c) まで一般化させてしまえば、誰でもかなり理解しやすい関数になると思います。（それでも∠ABC であって ∠ACBではないとか書く必要あると思いますが）
計算した角度をどのように利用するか？は IsMeageable 側だけを見ればわかるようになっていると有難いです。
|
最後のコメント以外を反映しました。 |
|
doneフラグの更新も修正しました。 uniteしたフラグメントの全原子についてフラグを立てるように変更しています。 |
src/MoleculeToFragments.cc
Outdated
| uf.unite(a, b); | ||
| for (int id_a : atomids_subst_a) done[id_a] = true; | ||
| for (int id_b : atomids_subst_b) done[id_b] = true; |
There was a problem hiding this comment.
うーーーーーん、どうしよう。これをやると、例えばベンゼン環に3つのクロロがくっついている時に、 c1(Cl)cc(*)cc(*)c1 と Cl* と Cl* になった時点で併合が止まってしまいます。
|
#変更内容
|
| @@ -53,6 +53,13 @@ namespace { | |||
| return ( c.end() != std::find(c.begin(), c.end(), v) ); | |||
| } | |||
|
|
|||
| /** | |||
| * @brief Extract the substructure of the original molecule by the atom IDs. | |||
There was a problem hiding this comment.
atomとともにbondも抜き出されることを明記しておきたいです。両端の原子が選ばれているbondもextractされる、というような表現になりますかね？
| * @param mol Molecule to be rotated | ||
| * @param bond_id Bond ID that serves as the axis of rotation | ||
| * @param th Rotation angle in radian | ||
| * @param rotate_is_id1 Whether fragment is rotated around atom_id1 or atom_id2 (default: true) |
There was a problem hiding this comment.
rotateは動詞なので、is はおかしいと思います ＆ rotateするのはatom id1 が属するフラグメントであって原子 atom_id1 自体ではない（こいつは回転しても座標は変わらない）と思います。
@param rotate_atom1_side
Specifies which substructure to rotate around the bond. If set to true, the substructure containingatom_id1is rotated; otherwise, the substructure containingatom_id2is rotated.
…chatGPT先生にお願いしたら結構長くなっちゃいました。
| new_mol.translate(mol.getCenter() - new_mol.getCenter()); | ||
|
|
||
| return new_mol; | ||
| } | ||
|
|
||
| bool IsMergeable(const Molecule &mol, const vector<int> &atomids_subst_a, const vector<int> &atomids_subst_b) { | ||
| // try to unite atoms a and b | ||
| /** |
There was a problem hiding this comment.
構造変化だけでなく、そもそも分子が違う場合も含まれてしまっているので、関数名から想像されるものより広い領域をカバーしてしまっていると思います。また、structure という表現は 2D-structureか3D-structureか？などの曖昧性も含みます。
are_different_posesとすることを提案します。- poseはconformer + 並進移動 + 回転移動、と考えています。今回の場合、構造の重ね合わせなどを行わずに座標をチェックしているので、分子全体の並進移動や回転移動によっても、trueが返されることになり、poseと呼ぶのが最もふさわしいと考えました。
- 異なる分子が入力された場合はエラーを出した方が安全です。
| @@ -72,21 +79,29 @@ namespace { | |||
| return temp_mol; | |||
| } | |||
|
|
|||
| Molecule bond_rotate(const Molecule& mol, int bond_id, fltype th) { | |||
There was a problem hiding this comment.
めちゃくちゃ今更なんですが、th は threshold に誤認するので theta とするのはいかがでしょうか…？
変更内容
IsMergeable()から関数切り出しIsNewRing()として切り出しIsNewRing()が必要な場面が想像できないため、そもそも不要かもしれない。個人的に
id_1とid_2の所属フラグメント判定やcalc_max_angle()の入出力が綺麗ではないと思ってはいますが、他の箇所でもご指摘あればよろしくお願いいたします。