Skip to content

Conversation

@tjjarvinen
Copy link
Collaborator

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
Copy link
Member

cortner commented Sep 21, 2023

so is this 100% backward compatible?

@tjjarvinen
Copy link
Collaborator Author

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

@wcwitt
Copy link
Collaborator

wcwitt commented Sep 22, 2023

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
Copy link
Member

cortner commented Sep 22, 2023

good idea to try them side by side

@wcwitt
Copy link
Collaborator

wcwitt commented Sep 22, 2023

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.

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.

3 participants