-
Notifications
You must be signed in to change notification settings - Fork 6
Copy States Optimization #297
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
Copy States Optimization #297
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 94.49% 94.64% +0.14%
==========================================
Files 9 9
Lines 654 728 +74
==========================================
+ Hits 618 689 +71
- Misses 36 39 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8ac5abb to
273cde9
Compare
tkoskela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Chenge, I hope you are doing well. Sorry it's taken so long to review your code after marking the report. I think this is very good work and should definitely get merged into the upstream repo.
I'd like to have a few clarifying comments in some places that I've highlighted in my review comments. I think overall the optimisations you made should be the default behaviour, rather than something the user has to switch on. This would especially simplify the copy_states! and copy_states_dedup! functions that duplicate some code at the moment.
If Matt and Mose could also take a look at this, that would be great!
tkoskela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I noticed while going through the GitHub Actions logs. Can we suppress the output from HiGHS in tests? The output from julia-runtest is long
matt-graham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Angeladadd - this looks great in general. I've added a couple of minor comments / suggestions, but these are not blockers.
8a821d9 to
ab1fda5
Compare
Co-authored-by: Matt Graham <[email protected]>
Co-authored-by: Matt Graham <[email protected]>
matt-graham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Angeladadd for the further updates. Just to say from my perspective this looks good to merge but will let @tkoskela and @giordano a chance to also look over things
tkoskela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now! Thanks for all your hard work @Angeladadd
Description
This PR introduces a two-phase optimisation to address communication bottlenecks in the copy_states routine during distributed resampling:
Issue
#116
Testing