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

New assembly #69

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

New assembly #69

wants to merge 3 commits into from

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