Changes to how the parameter object is scattered in dask#1048
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1048 +/- ##
==========================================
- Coverage 72.67% 72.61% -0.06%
==========================================
Files 20 20
Lines 5068 5076 +8
==========================================
+ Hits 3683 3686 +3
- Misses 1385 1390 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the parallel computation approach in the OG-Core economic modeling library by migrating from dask delayed objects to dask futures for better performance and resource management.
- Replaces delayed objects with client.submit() futures for parallel execution
- Optimizes parameter scattering by moving the Specifications object outside the TPI iteration loop
- Implements proper serial fallback logic when no dask client is available
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ogcore/TPI.py | Refactors TPI loop to use futures instead of delayed objects and moves parameter scattering outside the loop |
| ogcore/SS.py | Updates steady-state solver to use futures pattern with proper serial fallback implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| method=p.FOC_root_method, | ||
| tol=MINIMIZER_TOL, | ||
| ) | ||
| def solve_for_j( |
There was a problem hiding this comment.
The function solve_for_j is now defined inside inner_loop but is called in both parallel and serial execution paths. Consider moving this function definition outside of inner_loop to improve code organization and avoid redefining the same function multiple times during execution.
|
@jdebacker. We definitely want to update the version with this PR. Please make the changes in |
|
@jdebacker. I am just running these changes in the |
|
@rickecon I've updated the files for a new release. I think this PR is ready. |
|
@jdebacker. Performance stats from my local run of Baseline steady-state (2 min)
Baseline transition path equilibrium (23 min, 2.9 sec)
Reform steady-state (1 min)
Reform transition path equilibrium (26 min, 6.8 sec)
|
This PR makes some changes to how the parameter object is scattered and referenced by dask futures calls in
SS.pyandTPI.py. In particular:Specificationsobject,p, is scattered outside of the TPI loop since it's not changed in the while loop (so only need to define once)