-
Notifications
You must be signed in to change notification settings - Fork 3
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
Rework trait system #9
Merged
Merged
Conversation
This file contains 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
This comment was marked as outdated.
This comment was marked as outdated.
Started from scratch, hopefully this will allow for |
This comment was marked as outdated.
This comment was marked as outdated.
hhirtz
force-pushed
the
trait-rework
branch
2 times, most recently
from
March 23, 2022 13:57
e31c07f
to
ffcdcd0
Compare
This simplifies the family of traits from before into one "Partition" trait that is generic over the input of the algorithm. Thus, algorithms can partition points, weights or graphs without implementing several traits. The partition function can now mutate algorithm state, which is useful for PRNG state inside coupe::Random. Algorithms can now return errors instead of panicking, which is more FFI-friendly. Algorithms can now return arbitrary information (e.g. coupe::VnBest returns its number of iterations), which removes the need for coupe::RunInfo. == Limitations == Some implementations cannot be as generic as we want (e.g. Kmeans) At first, I wanted k-means to implement `Partition` for any `P: AsRef<[PointND<D>]>` and `W: AsRef<[f64]>` (so that it can also be used with a &Vec<_>), but rust doesn't accept those kind of "loose" constraints on const-generic parameters. I suppose this will do for now. Surprisingly, RCB, which has looser constraints, is fine. == Misc changes == All algorithm structs have been moved in their respective module, to declutter lib.rs. Constructors/Builder patterns have been removed since they didn't add much (and a fn new with 5 parameters is not that readable). It's simpler to have a Default impl and let the user do Algo { specific_param, ..Algo::default() }.
All of its items were exported anyway
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Trait changes
There is now only one
Partition
trait, which exposes apartition
function and two types:coupe/src/lib.rs
Lines 46 to 66 in 5521e9c
Algorithms' input is represented by the generic type
M
(e.g.&'a [Point2D]
, orW: IntoIterator
or even()
for the random partitioning algorithm).The function now can now mutate algorithm state. I don't know if it's something we want or not, but
Random
was using aRefCell
for the prng state.The
Error
type will be used instead of panics. I don't have modified all algorithms yet, and would rather do that in another PR.Example usage
Implementation of VNBest, which is currently the only algorithm to return diagnostic data (metadata, see #34):
coupe/src/algorithms/vn/best.rs
Lines 151 to 170 in 5521e9c
Example usage of RCB:
Limitations
Some implementations cannot be as generic as we'd want, for example k-means:
coupe/src/algorithms/k_means.rs
Line 824 in 5521e9c
At first, I wanted k-means to implement
Partition
for anyP: AsRef<[PointND<D>]>
andW: AsRef<[f64]>
(so that it can also be used with a&Vec<_>
), but rust doesn't accept those kind of "loose" constraints on const-generic parameters.I suppose this will do for now. Surprisingly, RCB, which has looser constraints, is fine:
coupe/src/algorithms/recursive_bisection.rs
Lines 593 to 602 in 5521e9c
Misc changes
All algorithm structs have been moved in their respective module, to declutter
lib.rs
.Constructors/Builder patterns have been removed since they didn't add much (and a
fn new
with 5 parameters is not that readable). It's simpler to have aDefault
impl and let the user doAlgo { specific_param, ..Algo::default() }
.