Skip to content
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

Remove nalgebra from api #6

Merged
merged 4 commits into from
Sep 24, 2021
Merged

Remove nalgebra from api #6

merged 4 commits into from
Sep 24, 2021

Conversation

hhirtz
Copy link
Member

@hhirtz hhirtz commented Sep 17, 2021

Simpler version of #3 where nalgebra's const-generic types are used instead of creating our own.

As said in the linked PR, I think this'll need to wait for generic_const_exprs to be used in nalgebra.

Also don't enable the default "alga" feature for sprs, since alga is
unmaintained and to-be-replaced by simba:

https://github.com/dimforge/alga/tree/dev/alga#readme
by using nalgebra::SVector and SMatrix, which use const generics.
Copy link
Member Author

@hhirtz hhirtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several issues with the deps update (see below). Otherwise, most nalgebra trait bounds were able to be replaced by const generics.

@@ -126,7 +126,7 @@ mod tests {
let w = [1, 2, 3, 4, 5, 6];
let mut part = vec![0 as usize; w.len()];
let imb_ini = imbalance::imbalance(2, &part, &w);
vn_best_mono(&mut part, &w, 2);
vn_best_mono::<u32>(&mut part, &w, 2);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VnBest now requires its generic type to be specified, otherwise rust fails with this weird, seemingly unrelated error:

error[E0275]: overflow evaluating the requirement `for<'r> &'r OPoint<_, _>: Sub`
   --> src/lib.rs:756:9
    |
756 |         algorithms::vn::best::vn_best_mono(&mut ids, &weights_real, self.num_parts);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    | 
   ::: src/algorithms/vn/best.rs:11:8
    |
11  | pub fn vn_best_mono<T>(partition: &mut [usize], criterion: &[T], nb_parts: usize) -> RunInfo
    |        ------------ required by a bound in this
...
14  |     for<'a> &'a T: Sub<Output = T>,
    |                    --------------- required by this bound in `vn_best_mono`
    |
    = help: consider adding a `#![recursion_limit="256"]` attribute to your crate (`coupe`)
    = note: required because of the requirements on the impl of `for<'r> Sub` for `&'r CsMatBase<OPoint<_, _>, _, _, _, _, _>`
    = note: 125 redundant requirements hidden
    = note: required because of the requirements on the impl of `for<'a> Sub` for `&'a CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<CsMatBase<OPoint<_, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>, _, _, _, _, _>`

Copy link
Member Author

@hhirtz hhirtz Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on sparsemat/sprs#295

@@ -461,7 +461,7 @@ mod tests {
let mbr = Mbr::from_points(&points);
let aspect_ratio = mbr.aspect_ratio();

relative_eq!(aspect_ratio, 4.);
assert_relative_eq!(aspect_ratio, 4.);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approx has labeled relative_eq as #[must_use] and, indeed, it's assert_relative_eq! that must be used here.

The following tests now fail:

    geometry::tests::test_inertia_vector_3d
    geometry::tests::test_mbr_2d
    geometry::tests::test_mbr_3d
    geometry::tests::test_mbr_distance_to_point_2d
    geometry::tests::test_mbr_distance_to_point_3d

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened issue #10

@hhirtz hhirtz merged commit 7deb4c6 into master Sep 24, 2021
@hhirtz hhirtz deleted the remove-nalgebra-from-api branch September 28, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant