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

decimal::d128 with alga::general::real #49

Open
snowkeep opened this issue Sep 19, 2018 · 7 comments
Open

decimal::d128 with alga::general::real #49

snowkeep opened this issue Sep 19, 2018 · 7 comments

Comments

@snowkeep
Copy link

I suspect this is more of a usage error than a bug, but can't anywhere else to go for help.

I'm trying to use the ncollision2d and decimal crates, and keep getting this error:

error[E0277]: the trait bound `decimal::d128: alga::general::Real` is not satisfied
  --> src/misc.rs:86:3
   |
86 |   pub cuboid: Cuboid<d128>,
   |   ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `alga::general::Real` is not implemented for `decimal::d128`
   |
   = note: required by `ncollide2d::shape::Cuboid`

Among others, I have this in my Cargo.toml:

[dependencies]
ncollide2d = "0.17.1"
decimal = "2.0.4"

[dependencies.alga]
version = "0.7.1"
features = ["decimal"]

As far as I can tell, that should enable the Real trait for decimal, but it isn't working. Is this a problem with alga? If not, can you tell me what I'm missing?

Thanks.

@WaDelma
Copy link
Contributor

WaDelma commented Sep 19, 2018

If you check Cargo.lock is there only one version of alga there? It could be that ncollide has different version which would cause this.

@snowkeep
Copy link
Author

Just 0.7.1. I also tried deleting the Cargo.lock, and running cargo clean, cargo build and confirmed that only the one version gets compiled.

@sebcrozet
Copy link
Member

@snowkeep There are two issues here:

  1. It appears the #[cfg] guards related to d128 on alga are plain wrong. So enabling the decimal feature actually has no effect. I will fix this now.
  2. Unfortunately the Real trait cannot be implemented for d128 because of missing trait implementations on the decimal crate. See Implement num traits for d128 alkis/decimal#21 So, even after fixing the first issue mentioned here, the Real implementation will not be available as long as the num traits are not implemented for d128. For the record, the missing traits seem to be Bounded, Signed, FromPrimitive so they should not be too difficult to implement.

sebcrozet added a commit that referenced this issue Sep 20, 2018
The Real trait requires some work on the `decimal` trait first.
Related to #49.
sebcrozet added a commit that referenced this issue Sep 20, 2018
The Real trait requires some work on the `decimal` trait first.
Related to #49.
@snowkeep
Copy link
Author

@sebcrozet. Thank you.
I can't attempt to add the traits to d128 immediately, so I'll work on the rounding errors with f64 types in my code, for now.
But I do intend to create a test-case and work on making d128 compatible. It's too useful to not have it.

sebcrozet added a commit that referenced this issue Sep 20, 2018
The Real trait requires some work on the `decimal` trait first.
Related to #49.
@snowkeep
Copy link
Author

I've added what looked like the necessary traits (num_traits::{Bounded, Signed, FromPrimitive and a hanful of other} and approx::{UlpsEq, RelativeEq and AbsDiffEq} in a branch called alga_support. I punted on from_str_radix for now (returns Ok(d128!(0.0)) - it's the most complicated method, by far, and I don't see a need for it, so want to test everything else before taking the time. Tests need to be added, as well.
I modified a clone of alga to point to this. Still a couple of issues. It looks like alga::general::{AbstractField, SubsetOfdec128::dec128, Lattice} traits are missing. I'll keep looking, but am going to need some help.
If anybody wants to look at it, my test case is here: https://github.com/snowkeep/d128_alga. Watch out for the relative paths to a local alga and decimal in the Cargo.toml.

@sebcrozet
Copy link
Member

sebcrozet commented Oct 1, 2018

Given the changes you've proposed on the decimal crate, I'm reopening this. I will see now what can be done using your changes.

@sebcrozet sebcrozet reopened this Oct 1, 2018
@sebcrozet
Copy link
Member

sebcrozet commented Oct 1, 2018

Thank you for your changes on decimal @snowkeep. Unfortunately, a few features (which are likely hard to implement) are still missing. You will find Real implemented for d128 on that branch: https://github.com/rustsim/alga/tree/d128 (and you need to enable alga's decimal feature).

Unfortunately, lots of methods of the Real trait are currently unimplemented!() because I could not find the corresponding methods on the decimal crate. Most of them are not actually necessary for ncollide itself, but some are essential: sin, cos, tan, asin, acos, atan2, sqrt, floor, ceil, round.

Also all the mathematical constants appear to be missing but I guess we could just convert constants from f64 to d128 for a first easy implementation (for example d128!(f64::consts::PI) for the ::pi() constant).

Finally, I could not find a way to convert from d128 to lower-precision floats like f64 so the SubsetOf<d128> contains an unimplemented!() too (but this should not be used by ncollide either).

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

3 participants