Optimize conformer comparison algorithm #765

JintaoWu98 · 2024-08-20T05:23:42Z

The previous conformer comparison algorithm required a distance matrix (d-matrix) for operations. In this update, we introduce a new variable, fl_distance, which represents the distance between the first and last elements. This method provides a less costly preliminary comparison before invoking the d-matrix comparison. Now, the d-matrix comparison will only be executed when necessary, optimizing the overall efficiency of the algorithm.

codecov · 2024-08-20T07:10:26Z

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.98%. Comparing base (8ba45be) to head (693b2f3).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
arc/species/conformers.py	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #765      +/-   ##
==========================================
+ Coverage   73.95%   73.98%   +0.02%     
==========================================
  Files         101      101              
  Lines       27777    27804      +27     
  Branches     5817     5825       +8     
==========================================
+ Hits        20542    20570      +28     
+ Misses       5774     5773       -1     
  Partials     1461     1461

Flag	Coverage Δ
unittests	`73.98% <96.96%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alongd

Thanks! I added my initial review.

alongd · 2024-08-20T06:01:31Z

arc/species/converter.py

+    fl_distance1 = np.linalg.norm(np.array(xyz1['coords'][0]) - np.array(xyz1['coords'][-1]))
+    if fl_distance2 is None:
+        fl_distance2 = np.linalg.norm(np.array(xyz2['coords'][0]) - np.array(xyz2['coords'][-1]))
+    if abs(fl_distance1-fl_distance2)/fl_distance1>rtol:


please add a space before and after operators

alongd · 2024-08-20T06:02:15Z

arc/species/converter.py

+        fl_distance2 = np.linalg.norm(np.array(xyz2['coords'][0]) - np.array(xyz2['coords'][-1]))
+    if abs(fl_distance1-fl_distance2)/fl_distance1>rtol:
+        return fl_distance1, fl_distance2, dmat1, dmat2
+    if dmat1 is None:


at this point we know for certain that dmat1 is None

alongd · 2024-08-21T07:58:03Z

arc/species/conformers.py

-                    exists = True
+                dmat1, fl_distance1 = None, None
+                for conf in new_conformers + newest_conformer_list:
+                    conf['dmat'] = conf.get('dmat')


can you explain why this is needed?

I think otherwise we may get into a case where conf dictionary doesn't have dmat at the beginning.

alongd · 2024-08-21T08:01:37Z

arc/species/conformers.py

+                                                                                          fl_distance2=conf['fl_distance'])
+                    conf['fl_distance'] = fl_distance2 if conf['fl_distance'] is None else conf['fl_distance']
+                    conf['dmat'] = dmat2 if conf['dmat'] is None else conf['dmat']
+                    if dmat1 is None:


I understand that if dmat1 is None, this implies that the fl_distance was too large. But this code is not self-explanatory, therefore hard to maintain and understand.
I recommend to pass the two conformer dictionaries to compare_confs_fl, then have compare_confs_fl return three things: whether the conformers are similar or not and the two updated conformer dictionaries. The we update our confs with the returned dictionaries

Thanks, I agree. I modified compare_confs_fl with your advice. However, it's a bit inconvenient to pass both conf dicts under conformers_combinations_by_lowest_conformer, since we are looping xyz, as shown here.
However, if you would like me to modify it further, please let me know.

alongd

Thanks! Please see some comments

alongd · 2024-08-31T15:36:36Z

arc/species/converter.py

+    fl_distance1 = np.linalg.norm(np.array(xyz1['coords'][0]) - np.array(xyz1['coords'][-1]))
+    if conf2['fl_distance'] is None:
+        conf2['fl_distance'] = np.linalg.norm(np.array(xyz2['coords'][0]) - np.array(xyz2['coords'][-1]))
+    if abs(fl_distance1-conf2['fl_distance']) / fl_distance1 > rtol:


You can use np.isclose here

alongd · 2024-08-31T15:38:16Z

arc/species/converter.py

+    if abs(fl_distance1-conf2['fl_distance']) / fl_distance1 > rtol:
+        return fl_distance1, dmat1, conf2, simliar
+    simliar = True
+    dmat1 = xyz_to_dmat(xyz1)


why do we compute dmat1 in this function?
This function also doesn't use conf2['dmat'], maybe we can simplify it?

The purpose of this function compare_confs_fl, is as following:

To compare two Cartesian coordinates representing conformers by examining the distances between the first and last atoms.

Distance matrices are computed and returned only if these distances are identical.

Considering potential modifications to the second purpose of this process, here are three options:

Rename the function to compare_confs_pre to indicate that it performs a preliminary, quicker comparison before the more resource-intensive compare_confs calculation.

Create a new function to do this calculation.

Merge this part to compare_confs, though feasible, I do not recommend this due to the complexity it would add, especially in returning the variable conf2, which could complicate the primary function.

alongd · 2024-08-31T15:42:27Z

arc/species/conformers.py

                if xyz is not None and energy is not None:
                    conformer = {'index': len_conformers + len(new_conformers) + len(newest_conformer_list),
                                 'xyz': xyz,
                                 'FF energy': round(energy, 3),
                                 'source': f'Changing dihedrals on most stable conformer, iteration {i}',
                                 'torsion': tor,
-                                 'dihedral': round(dihedral, 2)}
+                                 'dihedral': round(dihedral, 2),
+                                 'dmat': dmat1 if dmat1 is not None else None,


If dmat1 either has a value or can be None, and you want its value or else None, then considering just keeping 'dmat': dmat1 instead of the entire line. Same for the line below

alongd · 2024-08-31T15:43:39Z

arc/species/conformers.py

-        if index > 0 and not any([converter.compare_confs(lowest_conf['xyz'], conformer_list[index]['xyz'])
-                                  for lowest_conf in lowest_confs]):
-            lowest_confs.append(conformer_list[index])
+        if index>0:


add a space before and after operators

Compare two Cartesian coordinates representing conformers using first and last atom distances. If the `fl_distances` are the similar, the distance matrices are computed and returned.

If we don't need the conversion, meaning `dmat1` and `dmat2` are both given, we can start the subsequent steps directly.

arc/species/conformers.py

Save `fl_distance` and `dmat` to each of the `conformer` for efficient processing.

alongd

Looking good. Thanks!

github-actions bot added Module: Conformers Module: Converter labels Aug 20, 2024

alongd reviewed Aug 21, 2024

View reviewed changes

JintaoWu98 force-pushed the compare_confs_opt branch 4 times, most recently from 8a7b5b0 to 89ce5f5 Compare August 21, 2024 10:48

alongd reviewed Aug 31, 2024

View reviewed changes

JintaoWu98 force-pushed the compare_confs_opt branch 4 times, most recently from 4f2177a to 847440f Compare September 1, 2024 06:47

JintaoWu98 added 2 commits September 1, 2024 16:50

Define compare_confs_fl as a cheaper preprocess for compare_confs

ab20425

Compare two Cartesian coordinates representing conformers using first and last atom distances. If the `fl_distances` are the similar, the distance matrices are computed and returned.

Modify compare_confs for different cases

2418f64

If we don't need the conversion, meaning `dmat1` and `dmat2` are both given, we can start the subsequent steps directly.

JintaoWu98 force-pushed the compare_confs_opt branch from 847440f to b5ad9e0 Compare September 1, 2024 08:50

JintaoWu98 commented Sep 1, 2024

View reviewed changes

arc/species/conformers.py Show resolved Hide resolved

Modify the conformer comparison logic for efficiency

693b2f3

Save `fl_distance` and `dmat` to each of the `conformer` for efficient processing.

JintaoWu98 force-pushed the compare_confs_opt branch from b5ad9e0 to 693b2f3 Compare September 1, 2024 08:56

alongd approved these changes Sep 2, 2024

View reviewed changes

alongd merged commit 9e5c210 into main Sep 2, 2024
7 checks passed

alongd deleted the compare_confs_opt branch September 2, 2024 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize conformer comparison algorithm #765

Optimize conformer comparison algorithm #765

JintaoWu98 commented Aug 20, 2024

codecov bot commented Aug 20, 2024 •

edited

Loading

alongd left a comment

alongd Aug 20, 2024

alongd Aug 20, 2024

alongd Aug 21, 2024

JintaoWu98 Aug 21, 2024

alongd Aug 21, 2024

JintaoWu98 Aug 21, 2024 •

edited

Loading

alongd left a comment

alongd Aug 31, 2024

alongd Aug 31, 2024

JintaoWu98 Sep 1, 2024

alongd Sep 2, 2024

alongd Aug 31, 2024

alongd Aug 31, 2024

alongd left a comment

Optimize conformer comparison algorithm #765

Optimize conformer comparison algorithm #765

Conversation

JintaoWu98 commented Aug 20, 2024

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JintaoWu98 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2024 •

edited

Loading

JintaoWu98 Aug 21, 2024 •

edited

Loading