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

Faster IDPP #302

Merged
merged 2 commits into from
Oct 29, 2023
Merged

Faster IDPP #302

merged 2 commits into from
Oct 29, 2023

Conversation

shoubhikraj
Copy link
Collaborator

@shoubhikraj shoubhikraj commented Sep 28, 2023

Remove parallelisation from IDPP. The overhead from creating new processes is larger than any gains from parallelisation.

Test for diels-alder with 200 IDPP images

  • MacOS - with parallelisation = 4.621 s, without parallelisation = 2.403 s
  • Windows - with parallelisation = 10.26 s, without parallelisation = 2.331 s

Checklist

  • The changes include an associated explanation of how/why
  • Test pass
  • Documentation has been updated
  • Changelog has been updated

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #302 (9f66730) into v1.4.1 (db7b03d) will increase coverage by 0.04%.
Report is 3 commits behind head on v1.4.1.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           v1.4.1     #302      +/-   ##
==========================================
+ Coverage   97.33%   97.37%   +0.04%     
==========================================
  Files         206      206              
  Lines       22957    23048      +91     
==========================================
+ Hits        22345    22443      +98     
+ Misses        612      605       -7     
Flag Coverage Δ
unittests 97.37% <100.00%> (+0.04%) ⬆️

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

Files Coverage Δ
autode/neb/original.py 97.53% <100.00%> (+1.43%) ⬆️

... and 145 files with indirect coverage changes

@shoubhikraj shoubhikraj changed the base branch from master to v1.4.1 September 28, 2023 13:35
@t-young31
Copy link
Member

I'm not convinced this gain is worth the additional code complexity. When a DFT calculation is O(minutes-hours) I'm not sure it makes sense to save O(seconds). Please convince me otherwise!

@shoubhikraj
Copy link
Collaborator Author

shoubhikraj commented Sep 28, 2023

@t-young31 That is true, but now the IDPP is being used for i-EIP (and I am writing code for freezing-string method where IDPP is called every iteration). The problem is more obvious on windows (updated timings now) where processes are created, so the overhead is even higher. For POSIX, forking is cheaper so the overhead is less. I honestly don't think it increases the complexity of code that much. It just moves one part of the code outside parallel loop. What do you think?

@t-young31
Copy link
Member

Agree – for testing speed and if called every iteration then it's worth the small amount more complexity

autode/neb/original.py Outdated Show resolved Hide resolved
@t-young31 t-young31 merged commit dafe17f into duartegroup:v1.4.1 Oct 29, 2023
17 checks passed
@shoubhikraj shoubhikraj deleted the idpp-noparr branch May 7, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants