refactor: Move MMCS implementation from dingo to volesti #113
Conversation
|
@vissarion @hariszaf sir, could you please review my PR |
There was a problem hiding this comment.
Thanks for the efforts! dingo is already using mmcs from volesti. The only thing left is the functions from the old implementation e.g. HPolytopeCPP::mmcs_step. As far as I understand those are intentionally left for testing if the cpp functions have the same behaviour.
| void get_polytope_as_matrices(double* new_A, double* new_b) const; | ||
|
|
||
| // the rounding() function | ||
| void apply_rounding(int rounding_method, double* new_A, double* new_b, double* T_matrix, |
There was a problem hiding this comment.
what is the reason for removing this?
| const Hpolytope HP_const = HP; | ||
| mmcs(HP_const, ess, S, total_ess, walk_len, rng); | ||
| samples = S.data(); | ||
| } else if (strcmp(method, "mmcs") == 0) { |
There was a problem hiding this comment.
could you please comment on the reason you rewrite this part?
There was a problem hiding this comment.
It was rewritten to redirect MMCS algorithm calls to volesti's implementation instead of using dingo's own code. This change was part of our larger refactoring effort to move all MMCS implementation details to volesti, which is the more appropriate home for sampling algorithms. The rewrite creates a cleaner separation of responsibilities between the libraries and eliminates code duplication, making future maintenance and improvements simpler.
There was a problem hiding this comment.
So the code before this PR was using the dingo's mmcs calls?
|
Hi @vfisikop! You are totally right that we kept some functions HPolytopeCPP::mmcs_step for example for debugging reasons. I took apply_rounding out because it wasn´t being use anymore after moving to MMCS volesti it was only use on previous implementation |
Currently, the Multiphase Monte Carlo Sampling (MMCS) algorithm is implemented in both dingo and volesti libraries, causing code duplication. This issue proposes moving the implementation entirely to volesti, where it belongs as a core algorithm.
Changes:
Refactor mmcs algorithm #93