search

ACEsuit / ACEfit.jl

Generic Codes for Fitting ACE models
MIT License
7 stars 6 forks source link

New assembly #69

Open tjjarvinen opened 11 months ago

tjjarvinen commented 11 months ago

This will add the new assemble function.

On my tests, the current fitting methods in ACEpotentials will get most of speed increases the new assemble brings in. But the AtomsBase framework should be a little bit faster due to extra threading and the evaluate_d improvement.

I removed the dependencies from the old assemble (ParallelDataTransfer and SharedArrays).

I left progress meter in place, even though it might not have use in most cases now.

I added kwargs option to assemble to transmit extra options to calculators. If you don't give any kwargs, the old use cases should work. But ideally in the future, when you implement feature_matrix, target_vector or weight_vector you should add kwargs... to the function call. There is no need to use kwargs to anywhere, only catch them.

cortner commented 11 months ago

so is this 100% backward compatible?

tjjarvinen commented 11 months ago

Yes, it should be 100% backwards compatible. But I have only tested with ACEpotentials.

wcwitt commented 11 months ago

Thanks! I am going to be a bit picky about this - apologies in advance.

Would you please add your assembly routine to the dev-assemble branch under a new name? Then we can collaborate in #70.

cortner commented 11 months ago

good idea to try them side by side

wcwitt commented 11 months ago

Here's (very briefly) my reasoning.

On my tests, the current fitting methods in ACEpotentials will get most of speed increases the new assemble brings in.

Do we agree that the (non-ACEbase) speedup is largely whether we have GC.gc() for each structure? Since your proposed routine removes this, we will need to test carefully on some large systems.