Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove infinite loop in linear polymer #3491

Merged
merged 3 commits into from
Feb 17, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Feb 17, 2020

Description of changes:

  • fix infinite loop in draw_polymer_positions() (regression from Fix polymer generation #3484)
  • remove fixed seed in the polymer_linear.py test that used to hide a bug in the previous implementation of draw_polymer_positions()

Cap the number of retries for monomer placement to `max_tries`,
as explained in the function documentation.
The `linear_polymer_positions()` function already has a retry
mechanism with a default value of max_tries=1000. Running the
test 10000 times with a random Mersenne Twister seed resulted
in no failure. A single run required 3 retries in one test
function, all the other runs placed a polymer on the first try
in every tests. 6 runs took 1 min instead of 250 ms to execute.
@jngrad jngrad added the BugFix label Feb 17, 2020
@jngrad jngrad added this to the Espresso 4.2 milestone Feb 17, 2020
@jngrad jngrad requested a review from fweik February 17, 2020 09:30
@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #3491 into python will increase coverage by <1%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3491    +/-   ##
=======================================
+ Coverage      87%     87%   +<1%     
=======================================
  Files         536     536            
  Lines       24400   24254   -146     
=======================================
- Hits        21250   21132   -118     
+ Misses       3150    3122    -28
Impacted Files Coverage Δ
src/core/polymer.cpp 93% <0%> (-3%) ⬇️
src/core/global.cpp 75% <0%> (-8%) ⬇️
...rc/core/electrostatics_magnetostatics/scafacos.cpp 65% <0%> (-6%) ⬇️
...re/bonded_interactions/bonded_interaction_data.cpp 88% <0%> (-3%) ⬇️
...re/electrostatics_magnetostatics/dipole_inline.hpp 91% <0%> (-2%) ⬇️
.../core/electrostatics_magnetostatics/p3m-common.cpp 89% <0%> (-2%) ⬇️
src/core/object-in-fluid/oif_global_forces.cpp 80% <0%> (-1%) ⬇️
src/core/grid_based_algorithms/lb.cpp 95% <0%> (-1%) ⬇️
src/core/particle_data.cpp 96% <0%> (-1%) ⬇️
...rc/utils/include/utils/math/triangle_functions.hpp 90% <0%> (-1%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cec08a2...ae2c1f8. Read the comment docs.

@@ -186,6 +187,11 @@ draw_polymer_positions(PartCfg &partCfg, int const n_polymers,
} else if (not positions[p].empty()) {
/* Go back one position and try again */
positions[p].pop_back();
rejections++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain why this is needed? Couldn't you just increase attempts_poly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, when the first one has fixed position. But there has to be a more elegant solution than this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this while loop doesn't increment attempts_poly, therefore in high density systems, you can get stuck in an infinite cycle of deleting the previous bead and adding a new one at the same position

@jngrad jngrad added the automerge Merge with kodiak label Feb 17, 2020
@kodiakhq kodiakhq bot merged commit f9ae0e8 into espressomd:python Feb 17, 2020
@jngrad jngrad modified the milestones: Espresso 4.2, Espresso 4.1.3 Feb 22, 2020
@jngrad jngrad deleted the fix-linear-polymer branch January 18, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak BugFix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants