Vn first multi#57
Draft
hhirtz wants to merge 2 commits into
Draft
Conversation
hhirtz
commented
Mar 24, 2022
Comment on lines
-59
to
-78
| if imbalance < new_imbalance { | ||
| if new_imbalance <= imbalance { | ||
| // The move decreases the partition imbalance. | ||
| imbalance = new_imbalance; | ||
| max_load = new_max_load; | ||
| partition[i] = q; | ||
| i_last = i; | ||
| } else { | ||
| // The move does not decrease the partition imbalance. | ||
| part_loads[p] += weights[i].clone(); | ||
| part_loads[q] = part_loads[p].clone() - weights[i].clone(); | ||
| continue; | ||
| } | ||
| imbalance = new_imbalance; | ||
| max_load = new_max_load; | ||
| partition[i] = q; | ||
| i_last = i; |
Member
Author
There was a problem hiding this comment.
This diff is so that vnfirst does not make the move when T::partial_cmp(imbalance, new_imbalance) returns None (imbalance < new_imbalance would return false, thus making the move)
However, this means vnfirst only makes a move when there are gains on all criterion at once, which is overly restrictive.
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
==========================================
+ Coverage 50.25% 50.62% +0.36%
==========================================
Files 42 42
Lines 7223 7143 -80
==========================================
- Hits 3630 3616 -14
+ Misses 3593 3527 -66
Continue to review full report at Codecov.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #54
It's just a proof of concept and not to be merged.
The current "mono" implementation of vnfirst can actually be used for multi-criteria partitioning, thanks to generic types.
This PR makes use of nalgebra's
SVector<T, const D: usize>, a wrapper around[T; D]that implements arithmetic operations andPartialOrdto effectively feed vector of numbers to the mono-criterion implementation of vnfirst.It also turns out there were some mistakes made during the port from the aire-de-jeu. Those are fixed I think but I didn't have the motivation to split these changes into separate commits.
Limitations
Notably the
SVectorimplementation ofPartialOrdreturns:Some(Greater)/Some(Lesser)/Some(Equal)when all elements of the first vector are greater/lesser/equal to those of the other,Noneotherwise,This is the reason why this if/else has been rewriten:
coupe/src/algorithms/vn/first.rs
Line 61 in 8d6e2c2
and also messes with
Itertools::minmaxcalls, I think, where we want the min and the max loads across all criteria.