Improved clarity and introduced a CalculationHelper for restarts

[codecov[bot]]

Codecov Report

Merging #128 (c69fb3d) into master (890747f) will increase coverage by 1.20%. The diff coverage is 86.29%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   84.82%   86.03%   +1.20%     
==========================================
  Files          68       69       +1     
  Lines        2702     2692      -10     
==========================================
+ Hits         2292     2316      +24     
+ Misses        410      376      -34     

Flag	Coverage Δ
unittests	86.03% <86.29%> (+1.20%)	:arrow_up:

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

Files Changed	Coverage Δ
carmm/run/workflows/react.py	86.34% <78.57%> (+9.63%)	:arrow_up:
carmm/run/workflows/helper.py	95.45% <95.45%> (ø)
carmm/run/aims_path.py	100.00% <100.00%> (ø)
examples/run_aims.py	95.65% <100.00%> (ø)
examples/run_workflows_ReactAims.py	100.00% <100.00%> (ø)

[ikowalec]

I got it to pass the tests including coverage, but the TODO must be resolved prior to merge

[logsdail]

Looks fine to me - only immediate question is whether imports should be in functions where used, not at the header for the entire class? I thought best practice is to import where needed, not globally.

[GaryLZW]

Hi Igor, I left some comments in the file too. Most of my suggestions are minor points. I think some if statements (i.e. exists(traj_name) and getsize(traj_name)) in the code can be combined with ‘and’. The second point is about moving back self.counter in the restart_setup instead, which could make the code more clear to read. But the code is still great with the current arrangement . Lastly, I spot a line that may be redundant (i.e. opt_restarts -= 1) Thanks!

[ikowalec]

Hi Igor, I left some comments in the file too. Most of my suggestions are minor points. I think some if statements (i.e. exists(traj_name) and getsize(traj_name)) in the code can be combined with ‘and’. The second point is about moving back self.counter in the restart_setup instead, which could make the code more clear to read. But the code is still great with the current arrangement . Lastly, I spot a line that may be redundant (i.e. opt_restarts -= 1) Thanks!

The issue with combining the two checks in the while loop is that an empty .traj file could terminate the loop prematurely. While the nested ifs don't look great, I am not sure how to improve this. The opt_restarts -= 1 ensures that any restarted geometry optimisation in the same folder (_0.traj, _1.traj, _2.traj) is checked. The adjustment to self.counter +=1 was moved into the _find_restart() to follow the while loop rather than after the function was called in restart_setup().

[ikowalec]

@logsdail Adjustments were made to the aims_path , particularly hawk and hawk-amd to start FHI-aims using srun followed by mpirun. Launching with srun prevents resource locking which allows tasks to run concurrently.

[logsdail]

@logsdail Adjustments were made to the aims_path , particularly hawk and hawk-amd to start FHI-aims using srun followed by mpirun. Launching with srun prevents resource locking which allows tasks to run concurrently.

That's all good with me - doesn't break others workflow.

[ikowalec]

Looks fine to me - only immediate question is whether imports should be in functions where used, not at the header for the entire class? I thought best practice is to import where needed, not globally.

imports now in respective functions

[ikowalec]

@logsdail @GaryLZW @GabrielBram @PavelStishenko It should be good to go now. Hawk and ARCHER2 jobs were tested in serial and concurrently.

[logsdail]

Says when I look at the reviews there are two outstanding, but none show for me. Are these with other reviewers?

[logsdail]

Says when I look at the reviews there are two outstanding, but none show for me. Are these with other reviewers?

Ignore - I think this was some weird delayed update on the web server. All good with me.