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

Initial work on a Vector trait #153

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

Chris00
Copy link
Contributor

@Chris00 Chris00 commented Mar 24, 2024

As discussed in #147, here is some preliminary work on the introduction of a Vector trait (for fit and cblas::level1). (It is better to have early feedback before doing more work! In particular a Stride trait must be developed to be able to add strides to any Vector container.)

Note that, as added benefits, the equality of lengths of inputs is now checked (no way of specifying a longer length than allocated) and complex numbers are handled correctly (and in a compatible way with their Rust standard representation).

@Chris00 Chris00 force-pushed the master branch 5 times, most recently from e48a6d3 to daf1b44 Compare March 25, 2024 09:44
src/cblas.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

Looks great to me! I'm not personally a user of this crate, so maybe try to check around you or in the community what they think about it?

@Chris00
Copy link
Contributor Author

Chris00 commented Apr 15, 2024

The code now triggers lots of “transmute used without annotations” which are unrelated to my changes.

There is still quite a bit of work to do (there are still stride which are unchecked).

@hugohp
Copy link
Contributor

hugohp commented Oct 8, 2024

Can you use stride crate instead of Slice/SliceMut?
https://docs.rs/stride/latest/stride/

@Chris00
Copy link
Contributor Author

Chris00 commented Nov 15, 2024

@hugohp Not sure what you have in mind since, here, the strides need not be const — Stride<T, S> requires S to be const. What we maybe could do is to implement Vector for Stride<T, S>.

Added benefits to the additional generality:
- all functions are bound checked;
- the complex numbers are handled correctly and are compatible with
  their standard Rust representation.
As this trait will be requested to be brought into scope to set
strides, it is good that the functions required to define its
capabilities do not conflict with methods that the type may
originally have.
An incorrect implementation of those traits may result in a
out-of-bounds access in the C code, whence invalidating safety.
In doing so, remove many std::mem::transmute, all type unsafe
conversions being handled in the "complex" module.
@Chris00 Chris00 force-pushed the master branch 2 times, most recently from d1fca8e to 766df07 Compare December 26, 2024 01:05
@Chris00 Chris00 force-pushed the master branch 2 times, most recently from d78eaf7 to 8d0701b Compare December 26, 2024 10:33
The benefit is that all examples are compiled with "cargo test" which
avoids bit rotting (without the need for CI).
@Chris00
Copy link
Contributor Author

Chris00 commented Dec 26, 2024

I've propagated the use of the Vector trait everywhere I saw fit. I've also used the standard Complex<f64> and Complex<f32> types (better for interoperability). The FFT modules use Vector<Complex<f64>> — much better API, IMHO.

I've also fixed documentation warnings and made the examples part of the crate (they thus are built with cargo test instead of requiring a separate step).

There is still work to do to make the API nicer but this is a first step. Let me know what you think.

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.

3 participants