Pbc dist bench #3475

Miro-Astore · 2021-12-06T01:59:55Z

Rewrote the whole benchmark since last pull request so made a new one.

Changes made in this Pull Request:

Added, for relevant functions, benchmarks for testing orthogonal and triclinic
PBC settings
Rewrote all distance benchmarks to use a universe of randomly generated
atoms. This lets the benchmarks scale to arbitrary sizes.

It also makes realistic atom densities which are important for the contact
matrix distance functions. If the universe is too dense then a matrix with
too many connections is made and the benchmark will time out. As is the
10 000 atom option requires a modification to timeout for asv to run the
benchmark.

I had to split the DistancesBench class into 2 classes to avoid
uncessary reruns of the benchmars which do not take pbc type as an argument. Hence I
created BetweenBench.

The 2 classes and the distribution of testing functions is

BetweenBench

time_between

DistancesBench
time_distance_array
time_distance_array_pre_allocated
time_self_distance_array
time_self_distance_array_pre_allocated
time_dist
time_dist_offsets
time_contact_matrix
time_contact_matrix_sparse

Splitting the classes like this was necessary because asv does not
allow us to easily skip certain combinations of parameters at the
function level, only in the setup()
function of the class.

…enchmarks The benchmarked distances functions now all test 3 PBC conditions: None, triclinic, orthogonal. Separated matrix contacts functions and simple distance functions into DistanceBench and ContactsBench classes to deal with time complexity issues.

Added, for relevant functions, benchmarks for testing orthogonal PBC settings and also removed 10000 atom benchmarks for contact matrix functions to avoid timeout failiures. Had to split the DistancesBench class into 3 classes to avoid failiures and uncessary reruns of the benchmark. These failiures were due to time outs when too many atoms were run in the benchmarks or distances.betweeen not taking box type as an argument. The latter case meant, to avoid failiure, we had to either split the classes out or run redundant benchmarks. The 3 classes and the distribution of testing functions is BetweenBench time_between DistancesBench time_distance_array time_distance_array_pre_allocated time_self_distance_array time_self_distance_array_pre_allocated time_dist time_dist_offsets ContactsBench time_contact_matrix time_contact_matrix_sparse Splitting the classes like this was necessary because asv does not allow us to easily skip certain combinations of parameters at the function level, only in the setup() function of the class.

… 3.7, changed back to 2.7

Took out redundant definitions of universe at the class level and innapropriate raising of Exceptions.

…ix timer with timeout

…rse contact matrix class

pep8speaks · 2021-12-06T01:59:58Z

Hello @Miro-Astore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file benchmarks/benchmarks/analysis/distances.py:

Line 206:5: E303 too many blank lines (2)
Line 344:5: E303 too many blank lines (2)
Line 355:5: E303 too many blank lines (2)
Line 375:1: W391 blank line at end of file

Comment last updated at 2023-10-27 23:16:00 UTC

codecov · 2021-12-06T02:19:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2197a2c) 93.40% compared to head (3d8be13) 93.37%.
Report is 27 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3475      +/-   ##
===========================================
- Coverage    93.40%   93.37%   -0.04%     
===========================================
  Files          170      184      +14     
  Lines        22255    23403    +1148     
  Branches      4071     4075       +4     
===========================================
+ Hits         20788    21853    +1065     
- Misses         951     1035      +84     
+ Partials       516      515       -1

see 20 files with indirect coverage changes

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

…en benchmark

benchmarks/benchmarks/analysis/distances.py

…more clear

Miro-Astore · 2022-01-31T06:06:32Z

Hi, just wondering if there's anything that still needs fixing for this pull request to be merged.

orbeckst

As I understand your changes, you changed the underlying data for DistanceBench — correct me if I am wrong. My concern with changing a benchmark case but keeping the name the same will break the history so looking at the time series of performances may now show an artificial increase or decrease in performance that has nothing to do with the code itself.

My opinion is that we should

keep old benchmarks as they are or retire them
add new benchmarks with new names

@tylerjereddy @hmacdope @IAlibay @richardjgowers if you have different opinions I am all ears. This looks like something we should be deciding now.

orbeckst · 2022-02-18T17:00:00Z

Sorry @Miro-Astore for the delay in reviewing. I think we have to first decide how we want to progress in cases like this one where the benchmarks themselves are changed.

Miro-Astore · 2022-02-18T18:22:39Z

No problem. I will be happy to adapt the code to whatever framework the development team decide on.

orbeckst · 2022-02-18T20:46:01Z

After a quick discussion on the developer list there are at least two developers (@richardjgowers and I) who think that we need to do what I said in my comment:

keep old benchmarks as they are
create new ones for anything new

Can you please change your PR in this way, @Miro-Astore ? Thank you very much! (I do look forward to seeing the results!)

orbeckst · 2022-02-18T20:47:00Z

Please ping me when you need me to review again, I try to do it reasonably quickly.

orbeckst · 2022-02-18T20:48:01Z

@hmacdope if you have time to look at any updates then that would also be very helpful.

hmacdope · 2022-02-18T22:22:17Z

@hmacdope if you have time to look at any updates then that would also be very helpful.

I'll get onto it asap

hmacdope · 2022-02-19T23:08:38Z

@orbeckst @Miro-Astore agree that we probably need to keep original benchmark names as otherwise performance history is not really linear.

Miro-Astore · 2022-02-20T19:54:51Z

No problem. I'll get to it after the BPS meeting. If any of you are here I'll be sure to say hi!

hmacdope

Looking good in general , just a few things.

Also worth considering is having a benchmark with a significant portion of the coordinates outside the primary cell.

I could be wrong but for triclinic boxes I think this is slightly more expensive due to need to call _triclinic_pbc which has heaps of branching logic. For orthorhombic boxes I am fairly sure all the code paths are the same.

benchmarks/benchmarks/analysis/distances.py

…means that we will be running time_between 3 times instead of 1, without any difference because it doesn't take PBC types.

… class.

Miro-Astore · 2022-03-06T22:07:51Z

I think my latest commits need a bit more organisation sorry. I'll ping you when they're ready for reveiw.

Also worth considering is having a benchmark with a significant portion of the coordinates outside the primary cell.

Just a small note that I don't think this is a good idea because the original reason behind the bug in #2794 was because contact matrix calculations made massive matrices which cause MemoryErrors when lots of atoms are very close.

If we make atoms outside the box it could cause the same error because then they are wrapped into the smaller box they would closely overlap. I'll try and test it out at some point.

hmacdope · 2023-06-14T00:01:59Z

@Miro-Astore the diff seems enormous for some reason?

Miro-Astore · 2023-06-14T00:33:55Z

Sorry about that, I think I've done something silly and merged the current development branch because my current one was so old. It seems to have created a problem. Can you just look at the benchmarks/benchmarks/analysis/distances.py script or would you like me to try and fix the weird diffs ?

hmacdope · 2023-06-14T01:02:10Z

You will need to sort out diffs before merge is possible. @MDAnalysis/coredevs I forget what the fix is here.

orbeckst · 2023-06-14T01:03:08Z

Yes, please fix the patch. You also have conflicts. I'd go back to 087cb46. Then I'd try merging from GitHub. Once the merge is done cleanly, you can put the changes from 1eb6bdd back.

(Perhaps you can try a rebase ... but be careful with any history rewriting. If in doubt, ask first.)

…eenBench class." This reverts commit 3261f78.

Miro-Astore · 2023-06-15T20:05:12Z

Hi, sorry for the messy commit history. I think this is now ready?

github-actions · 2023-06-15T20:06:08Z

Linter Bot Results:

Hi @Miro-Astore! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

orbeckst

Very brief/terse review

The original DistancesBench needs to have the old data generation. If there's a need for a version with the new realistic density then make a copy.
You removed the time_calc_* methods. They should be added back, at least to DistancesBench.
Add .gitignore back.

orbeckst · 2023-06-15T21:36:20Z

.gitignore

@@ -1,54 +0,0 @@
-# Ignore python bytecoded files


please don't delete our .gitignore

benchmarks/benchmarks/analysis/distances.py

orbeckst · 2023-06-15T21:43:22Z

benchmarks/benchmarks/analysis/distances.py

+    def setup(self, num_atoms, pbc_type):
+
+        # This setup function is virtually identical to the one
+        # in the DistancesBench, we just have to do some extra 


DistancesBench2

orbeckst · 2023-06-15T21:43:36Z

benchmarks/benchmarks/analysis/distances.py

+                                 box=None)
+class DistancesBenchPBC(object):
+    """Benchmarks for MDAnalysis.analysis.distances
+    functions. 


orbeckst · 2023-06-15T21:44:17Z

benchmarks/benchmarks/analysis/distances.py

+        # calculating reasonable density for an atomistic system.
+        ex_universe = MDAnalysis.Universe(GRO)
+        density = ex_universe.coord.volume/ex_universe.atoms.n_atoms
+        desired_box_volume = universe_num_atoms*density


space around operators

orbeckst · 2023-06-15T21:44:23Z

benchmarks/benchmarks/analysis/distances.py

+        sides = [float(cube_side_length)]*3
+        ortho_angles = [float(90)]*3


space around operators

PEP 8 — space around operators

benchmarks/benchmarks/analysis/distances.py

Miro-Astore · 2023-10-28T00:04:39Z

Hi sorry this is taking so long, and for all the trouble. I think I'm ready for another round of reviews.

(accidentally made a giant commit again but i've reset it. Ping me if there's an issue. ) @orbeckst

Miro-Astore · 2023-11-20T22:18:09Z

Hi sorry this is taking so long, and for all the trouble. I think I'm ready for another round of reviews.

(accidentally made a giant commit again but i've reset it. Ping me if there's an issue. ) @orbeckst

bump

orbeckst

Looks very good; I only have one question (see comments) and please do some PEP8 formatting. Thanks!

orbeckst · 2023-11-21T01:37:55Z

.gitignore

Please don't delete lines from the file. They might be there for a reason.

If some clean-up is needed, do a separate mini PR.

orbeckst · 2023-11-21T02:06:43Z

benchmarks/benchmarks/analysis/distances.py

+        memory implementation of contact_matrix intended
+        for larger systems.
+        """
+        distances.contact_matrix(coord=self.ag1.positions,


Can you briefly explain how your changes address #2794 ? Does the numatom == 10000 case not time out anymore?

Perhaps we can just skip this case

from asv_runner.benchmarks.mark import skip_for_params @skip_for_params([(10000, 'numatoms')]) def time_contact_matrix_sparse(self, num_atoms): ...

orbeckst · 2023-11-21T02:07:59Z

benchmarks/benchmarks/analysis/distances.py

+        sides = [float(cube_side_length)]*3
+        ortho_angles = [float(90)]*3


PEP 8 — space around operators

orbeckst · 2023-11-21T02:08:07Z

benchmarks/benchmarks/analysis/distances.py

+
+        # Make fake universe with 3*num_atoms because some of our benchmarks
+        # require 3 atom groups.
+        universe_num_atoms = num_atoms*3


PEP 8 — space around operators

orbeckst · 2023-11-21T02:09:16Z

benchmarks/benchmarks/analysis/distances.py

+        density = ex_universe.coord.volume/ex_universe.atoms.n_atoms
+        desired_box_volume = universe_num_atoms*density


PEP 8 — space around operators

orbeckst · 2023-11-21T02:09:41Z

benchmarks/benchmarks/analysis/distances.py

+
+        # Make fake universe with 3*num_atoms because some of our benchmarks
+        # require 3 atom groups.
+        universe_num_atoms = num_atoms*3


PEP 8 — space around operators

orbeckst · 2023-11-21T02:09:50Z

benchmarks/benchmarks/analysis/distances.py

+        density = ex_universe.coord.volume/ex_universe.atoms.n_atoms
+        desired_box_volume = universe_num_atoms*density


PEP 8 — space around operators

orbeckst · 2023-11-21T02:09:59Z

benchmarks/benchmarks/analysis/distances.py

+        sides = [float(cube_side_length)]*3
+        ortho_angles = [float(90)]*3


PEP 8 — space around operators

orbeckst · 2023-11-21T05:16:43Z

@hmacdope can you also have a look if your previous concerns have been addressed?

Miro-Astore · 2024-01-31T18:51:11Z

@hmacdope
if i fix up the syntax errors is this ready to roll?

orbeckst · 2024-02-20T22:15:52Z

@Miro-Astore sorry ... hadn't seen you question.

If you address my comments then we should be good to go. Please ping me when you're done! Thank you!!

Miro-Astore added 9 commits August 24, 2021 17:29

changed version of python specified by asv.conf.json to 3.7

70f0fcf

Fixing mistake in asv.conf.json where version was changed from 2.7 to…

1a39f84

… 3.7, changed back to 2.7

Fixing formatting of distances benchmark

ca81568

Fixing up small errors in PBC pull request.

aea12a4

Took out redundant definitions of universe at the class level and innapropriate raising of Exceptions.

rehalled whole workflow, trying to fix bug in orthogonal contact matr…

20406b6

…ix timer with timeout

added timeout for distances bench class. prevents failiure of the spa…

a58d628

…rse contact matrix class

fixed formatting of distance.py benchmark

8f093fa

github-actions bot added the performance label Dec 6, 2021

fixed typo in testing, had 5000 instead of 10 000 for distances betwe…

4a97ee6

…en benchmark

tylerjereddy reviewed Dec 15, 2021

View reviewed changes

benchmarks/benchmarks/analysis/distances.py Outdated Show resolved Hide resolved

benchmarks/benchmarks/analysis/distances.py Outdated Show resolved Hide resolved

benchmarks/benchmarks/analysis/distances.py Outdated Show resolved Hide resolved

Miro-Astore added 2 commits January 18, 2022 14:32

made argument parsing to distance benchmarks for different pbc types …

8ff899d

…more clear

deleted redundant import of mda math

d8d42c8

orbeckst requested changes Feb 18, 2022

View reviewed changes

hmacdope requested changes Feb 22, 2022

View reviewed changes

benchmarks/benchmarks/analysis/distances.py Show resolved Hide resolved

benchmarks/benchmarks/analysis/distances.py Outdated Show resolved Hide resolved

benchmarks/benchmarks/analysis/distances.py Show resolved Hide resolved

Miro-Astore added 2 commits February 27, 2022 12:55

deleted BetweenBench class so everything is in one class again. Just …

b5e0c7d

…means that we will be running time_between 3 times instead of 1, without any difference because it doesn't take PBC types.

added time_between to DistancesBetween class and deleted BetweenBench…

3261f78

… class.

Miro-Astore added 5 commits June 14, 2023 14:26

rebasing, maybe ready for commit

411cb26

Merge branch 'detached_pbc_dist_bench' into pbc_dist_bench

7758290

Revert "added time_between to DistancesBetween class and deleted Betw…

ab4e2cb

…eenBench class." This reverts commit 3261f78.

reverted to a previous commit, ready for merge?

17cd7c9

Merge branch 'develop' into pbc_dist_bench

0c00e69

github-actions bot removed Component-Converters Continuous Integration Component-Topology Component-lib Component-Analysis Component-Core Component-Readers labels Jun 15, 2023

orbeckst requested changes Jun 15, 2023

View reviewed changes

Miro-Astore added 3 commits September 15, 2023 10:07

Merge branch 'MDAnalysis:develop' into pbc_dist_bench

36ea8d5

bond calculations added back to distances bench. and gitignore restored

3ef069a

fixed merge conflict gitignore

3d8be13

orbeckst requested changes Nov 21, 2023

View reviewed changes

orbeckst removed the close? Evaluate if issue/PR is stale and can be closed. label Nov 21, 2023

orbeckst self-assigned this Nov 21, 2023

		sides = [float(cube_side_length)]*3
		ortho_angles = [float(90)]*3

		density = ex_universe.coord.volume/ex_universe.atoms.n_atoms
		desired_box_volume = universe_num_atoms*density

Pbc dist bench #3475

Are you sure you want to change the base?

Pbc dist bench #3475

Conversation

Miro-Astore commented Dec 6, 2021 • edited Loading

pep8speaks commented Dec 6, 2021 • edited Loading

Comment last updated at 2023-10-27 23:16:00 UTC

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

Miro-Astore commented Jan 31, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Feb 18, 2022

Miro-Astore commented Feb 18, 2022

orbeckst commented Feb 18, 2022

orbeckst commented Feb 18, 2022

orbeckst commented Feb 18, 2022

hmacdope commented Feb 18, 2022

hmacdope commented Feb 19, 2022

Miro-Astore commented Feb 20, 2022

hmacdope left a comment

Choose a reason for hiding this comment

Miro-Astore commented Mar 6, 2022 • edited Loading

hmacdope commented Jun 14, 2023

Miro-Astore commented Jun 14, 2023 • edited Loading

hmacdope commented Jun 14, 2023

orbeckst commented Jun 14, 2023 • edited Loading

Miro-Astore commented Jun 15, 2023

github-actions bot commented Jun 15, 2023 • edited Loading

Linter Bot Results:

orbeckst 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

Miro-Astore commented Oct 28, 2023 • edited Loading

Miro-Astore commented Nov 20, 2023

orbeckst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Nov 21, 2023

Miro-Astore commented Jan 31, 2024

orbeckst commented Feb 20, 2024

Miro-Astore commented Dec 6, 2021 •

edited

Loading

pep8speaks commented Dec 6, 2021 •

edited

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading

Miro-Astore commented Mar 6, 2022 •

edited

Loading

Miro-Astore commented Jun 14, 2023 •

edited

Loading

orbeckst commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading

Miro-Astore commented Oct 28, 2023 •

edited

Loading