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

Multi-threading bugfix #158

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Multi-threading bugfix #158

merged 1 commit into from
Dec 14, 2022

Conversation

cortner
Copy link
Member

@cortner cortner commented Dec 7, 2022

forces! appears to produces incorrect results when used with ACE1.jl - only for pair potentials, not for many-body potentials. All quite weird. This PR replaces the lock with a numthreads x collection of forces, which might be preferrable anyhow. Not very elegant yet, but works and is needed to make ACE1.jl compatible with latest JuLIP.jl

@tjjarvinen -- I'd be grateful if you can take a look at this.

I'm tempted to just

  • merge this
  • tag a new patch release
  • so I can then next tag a new minor release for ACE1.jl.
  • and file an issue that this needs to be looked at again and cleaned up; but this can be done in the process of cleaning up multi-threading in general which we wanted to do anyhow.

@cortner
Copy link
Member Author

cortner commented Dec 13, 2022

@tjjarvinen -- Given I've asked you to take charge of fixing the multi-threading in JuLIP going forward, I don't want to merge this without your thoughts. I know it is a crappy fix, the question is whether you are happy to have this fix in there for now. Should we have a zoom call this week to discuss this?

@tjjarvinen
Copy link
Contributor

The operation seems to be a reduction, maybe using threaded sum operator from Folds.jl might be a better option.

I would also unite the num_threads == 1 and the multithreaded cases. Like, if the multithreading is robust then it should work well with what ever number threads there are, including one.

@cortner
Copy link
Member Author

cortner commented Dec 13, 2022

Main question is still whether to merge this now in order to move forward with ACE1, or work on a more elegant and general fix for multi-threading.

  • Re Folds.jl : if you are confident this is a good route to take, then please do try it out, maybe a separate PR?
  • Re the numthreads == 1 special case : I know it shouldn't be there. There were some cases a while ago where threading was not robust, hence we have this distinction. This was early days of threading in Julia, JuLIP and ACE, so it may well be fine to remove it now, I just never had the time to test this carefully. I propose to leave this until you rework the multi-threading in JuLIP.

@tjjarvinen
Copy link
Contributor

If it fixes the bug, then lets go ahead with it and implement more elegant version later.

@cortner cortner merged commit b3b2f41 into master Dec 14, 2022
@cortner cortner deleted the co/mtbug branch December 14, 2022 09:37
@cortner cortner mentioned this pull request Dec 14, 2022
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.

2 participants