MDSuite icon indicating copy to clipboard operation
MDSuite copied to clipboard

Improving RDF

Open PythonFZ opened this issue 4 years ago • 11 comments

I attempted to optimize the RDF code a little bit.

On an 108000 atoms system using

experiment.run_computation.RadialDistributionFunction(
    number_of_configurations=40, minibatch=32, batches=1, benchmark=True, correct_minibatch_batching=20
)

on an RTX2080 this code is about 20 % faster then the old code (0.76 GHz v. 0.91 GHz).

Main changes are:

  • optimized TF function usage
  • reduce redundancy
  • optimize batch size so the GPU usage keeps almost constant over the full computation and not only on the start

Additions:

  • benchmark kwarg
  • correct_minibatch_batching kwarg

Still missing

  • fix for large values on correct_minibatch_batching
  • documentation
  • small systems seem to fail

PythonFZ avatar Aug 02 '21 20:08 PythonFZ

Can you elaborate on the small systems seem to fail?

SamTov avatar Aug 02 '21 20:08 SamTov

Can you elaborate on the small systems seem to fail?

There are value errors and the RDFs look sometimes different to what they should look like.

PythonFZ avatar Aug 03 '21 06:08 PythonFZ

If you have any example that would be helpful. It's an odd problem.

SamTov avatar Aug 03 '21 06:08 SamTov

Is this PR ready to go?

SamTov avatar Aug 15 '21 13:08 SamTov

Unfortunately there are still issues with the Code that need to be resolved.

PythonFZ avatar Aug 15 '21 14:08 PythonFZ

Which ones? I'd like to bring this PR in so that I can run a black restructure on the code and merge the correctly formatted version. This could also wait until after all current PRs are done but it will make it a bit messier.

SamTov avatar Aug 15 '21 15:08 SamTov

If I remember correctly, running it with different parameters for correct_minibatch_batching gave different results

PythonFZ avatar Aug 15 '21 15:08 PythonFZ

I will look into it tonight

SamTov avatar Aug 15 '21 17:08 SamTov

I've run the RDF on a 1000 atom system with differing correct mini batch arguments and received the same values each time. I will now check the small system size comment.

SamTov avatar Aug 16 '21 11:08 SamTov

@PythonFZ I think this can really be closed now.

SamTov avatar Nov 17 '21 15:11 SamTov

@PythonFZ I think this can really be closed now.

I don't think it has to - the idea is still valid and would improve the RDF speed. It needs to be fixed and tested before merging, but I don't see any need to close this PR.

PythonFZ avatar Nov 17 '21 15:11 PythonFZ