Skip to content

Conversation

jimboid
Copy link
Member

@jimboid jimboid commented Aug 13, 2025

Summary

This PR is to include the parallel and temperature parameters to the waterentropy function call such that the default temperature set in waterentropy is overridden by that set in CE config file and also to cause water entropy to run in parallel.

Changes

Update function call to waterentropy with temperature and parallel params:

  • add in self.args.temperature as the temperature parameter
  • add in parallel=True to trigger parallel execution

Impact

This will make sure that temperature in waterentropy is set to the CE value and will trigger water entropy to run in parallel. We still need to parallelise CE but when we do the context should be passed also into WE so that we avoid the overhead of dual dask setups.

closes #125

This will trigger water entropy to run multithreaded. We should later percolate parallel worker threads into water entropy from CE to save overhead in setting up two lots of schedulers.
@jimboid jimboid linked an issue Aug 13, 2025 that may be closed by this pull request
Copy link
Contributor

@harryswift01 harryswift01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I can see that waterEntropy is now running in parallel, which should lead to a significant speedup in CodeEntropy. Thanks very much for implementing this!

Copy link
Collaborator

@jkalayan jkalayan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Included the extra dictionary produced from v1.2.0 of waterEntropy and it works on the arginine example trajectory. Parallel is switched on by default, which is fine, but can be included as an argument in the config file if requested in the future.

@jimboid
Copy link
Member Author

jimboid commented Aug 13, 2025

We can add it as a configurable parameter. I was initially going to leave it until it is clear whether or not we are implementing parallelism in CE. Since at this point I would initiate the worker topology in CE and pass the context into WE to override parallel setup. It's not clear yet if the slowness is coming from the conformational entropy added lots of pointless empty lists, and once fixed makes it fast enough to not bother. What does every one think? @jkalayan @harryswift01 @skfegan

Expose parallel/serial to config?
Set default to parallel or serial?

I'm not inclined to either way so if there is an opinion then please put it forward and we will implement.

Also I think I need to fix at least one test before I merge this.

@jimboid
Copy link
Member Author

jimboid commented Aug 13, 2025

Fixed the tests now

@jimboid jimboid marked this pull request as ready for review August 14, 2025 10:19
@jimboid jimboid merged commit 998f8cb into main Aug 14, 2025
7 checks passed
@jimboid jimboid deleted the 125-add-temperature-and-parallel-arguments-to-we branch August 14, 2025 10:19
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.

Add temperature and parallel arguments to WE
3 participants