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.
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.
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.
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 <matthew.m.graham@gmail.com>
Co-authored-by: Matt Graham <matthew.m.graham@gmail.com>
matt-graham
left a comment
There was a problem hiding this comment.
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.
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