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

Fine grained trait bounds #496

Open
elrnv opened this issue Nov 5, 2019 · 8 comments
Open

Fine grained trait bounds #496

elrnv opened this issue Nov 5, 2019 · 8 comments

Comments

@elrnv
Copy link
Contributor

elrnv commented Nov 5, 2019

This is possibly more of a question. Would it be of any interest to the users of cgmath to have more fine grained trait bounds on methods from std::ops to eliminate the need to add BaseNum or BaseFloat trait bounds into generic code in libraries that use cgmath? The motivation here is that it may perhaps be easier to swap in/out cgmath in libraries without modifying the trait bounds on all generic code. I know this may be a moot point at this moment, but seeing as cgmath aims to be one of the simpler and more light-weight matrix libs this might make sense here.

@elrnv
Copy link
Contributor Author

elrnv commented Nov 5, 2019

Another benefit of this is that libraries that implement lower level types like a custom floating point type (e.g. auto-diff, complex) would not need to implement BaseFloat, but will need to only target num_traits.

Edit: NVM about this, just realized this is handled automatically by the blanket impl of BaseFloat.

@kvark
Copy link
Collaborator

kvark commented Nov 19, 2019

I think an example would be useful here. What kind of stuff you'd be fine graining the trait bounds for?

@elrnv
Copy link
Contributor Author

elrnv commented Nov 21, 2019

Sure! Suppose I have bunch of code that looks like:

use num_traits::Zero;
fn make_some_mtx<T: Zero + Copy>() -> [[T;2];2] {
    let mtx = [[T::zero(); 2]; 2];
    // Maybe do something else with mtx.
    mtx
}

and for whatever reason I am using arrays, but now i want to use cgmath, so I write:

use num_traits::Zero;
use cgmath::Matrix2;
fn make_some_mtx<T: Zero + Copy>() -> Matrix2<T> {
    let mtx = Matrix::zero();
    // Maybe do something else with mtx.
    mtx
}

Which wont work since Matrix::zero() requires all of BaseFloat:

impl<S: BaseFloat> Zero for Matrix2<S> {
    #[inline]
    fn zero() -> Matrix2<S> {
        #[cfg_attr(rustfmt, rustfmt_skip)]
        Matrix2::new(
            S::zero(), S::zero(),
            S::zero(), S::zero(),
        )
    }

    #[inline]
    fn is_zero(&self) -> bool {
        ulps_eq!(self, &Self::zero())
    }
}

This means I have to also change my bounds:

use num_traits::Zero;
use cgmath::{BaseFloat, Matrix2};
fn make_some_mtx<T: BaseFloat>() -> Matrix2<T> {
    let mtx = Matrix::zero();
    // Maybe do something else with mtx.
    mtx
}

So for this particular example, the win isn't too much since I need the bound for ulps_eq as well, but there are other examples like SquareMatrix that require BaseFloat, although from_value or from_diagonal really should only need Zero and Copy.

(I personally would suggest a further step and have is_zero just do PartialEq with Zero, and have a separate is_approx_zero fn for measuring relative equivalence (in my personal experience float comparisons tend to be domain specific).)

But even having:

impl<S: Zero + Copy + UlpsEq> Zero for Matrix2<S> { ... }

would be nice. Again I understand this is opinionated since having a single trait define most scalars has its benefits, which is why it's more of a question.

@kvark
Copy link
Collaborator

kvark commented Nov 21, 2019

Thank you for providing the detailed examples! I agree, this would be a welcome feature.

@spearman
Copy link
Contributor

Would this make it easier to use fixed precision numbers with cgmath? Having square root would be enough to get stuff like magnitude and distances. I tried implementing enough of Float for the fixed crate and it seemed to work reasonably well, but there are some trait methods of Float that don't make sense for fixed precision numbers: https://gitlab.com/spearman/fixed/blob/8ed9f22ae221bbd7b1b8eb0c1ac8e31159b35c94/src/num.rs#L179-404

@elrnv
Copy link
Contributor Author

elrnv commented Nov 27, 2019

I think breaking down the trait bounds may help using types not already implementing all of BaseFloat, but I wasn’t intending to break apart num_traits::Float itself. I think that would be an interesting proposal for num_traits.

@mickvangelderen
Copy link
Contributor

mickvangelderen commented Feb 15, 2020

In this context, can/should cgmath::BaseNum and cgmath::BaseFloat be replaced with num_traits::PrimInt and num_traits::Float? Perhaps num_traits::PrimInt has too many restrictions and a trait representing a field would be more flexible? Taking the granularity to std::ops::Add for each function may make cgmath harder to use and changing performed computations may require adding or removing traits causing breaking changes.

I think we need to collect concrete examples like those provided by @spearman and @elrnv to figure out how to actually improve the situation. Also someone with a good understanding of the mathematical categorization can help grouping and naming behaviour (traits).

@elrnv
Copy link
Contributor Author

elrnv commented Feb 16, 2020 via email

elrnv added a commit to elrnv/cgmath that referenced this issue May 18, 2020
This makes the trait more flexible.
This contributes to rustgd#496.
kvark pushed a commit that referenced this issue May 23, 2020
This makes the trait more flexible.
This contributes to #496.
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

No branches or pull requests

4 participants