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

Fix 256: Makes instantiating DenseMatrix and others fallible #258

Closed

Conversation

EdmundsEcho
Copy link

Fixes issue 256

Several new tests were added to the src/linalg/basic/matrix.rs module.
All tests pass.

Fixes #

Checklist

  • My branch is up-to-date with development branch.
  • Everything works and tested on latest stable Rust.
  • [?] Coverage and Linting have been applied

Current behaviour

DenseMatrix relies on the user to properly input the dimensions of the matrix. In the event a mistake is made, the results will either be wrong, or generate an "index out of bounds" error.

New expected behaviour

Update the constructors for DenseMatrix. Set return value to Result<DenseMatrix, Failed> to reflect the fallible nature of the construction. This is true because the input relies on user input to align with the data provided. In other words, there is no way to infer and/or correct the user input when the implied matrix shape does not align with the data.

Change logs

Clearly a breaking change. However, this is a critically important change. There is no way around it. Before the change, it's possible to create references to invalid memory. This update leverages the current error generating infrastructure. In the event the user input is nonsensical (introduces a contradiction) given the actual data, a Failed::InputError is returned.

DenseMatrix, DenseMatrixView, DenseMatrixMutView, new and from_2d_array methods now validate the input and return a fallible result.

For future use, added a Failed::InvalidState to the enum. This may be useful in the development branch to safely assert and otherwise flag when critical to "be in" a valid state.

src/linalg/basic/matrix.rs was the core change. All of the tests, throughout the package were updated to reflect the new fallible construction. This was prolific, but simple: tagging .unwrap() to the constructions used in the tests.

@EdmundsEcho EdmundsEcho requested a review from Mec-iS as a code owner April 4, 2023 20:33
@EdmundsEcho EdmundsEcho changed the title initial commit Fixes issue 256 Apr 4, 2023
@EdmundsEcho EdmundsEcho changed the title Fixes issue 256 Fix 256: Makes instantiating DenseMatrix and others fallible Apr 4, 2023
Copy link
Collaborator

@morenol morenol left a comment

Choose a reason for hiding this comment

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

sounds good to me. Could you run cargo clippy --fix and cargo fmt to solve some linting issues?

Maybe this requires to bump smartcore to 0.4.0 @Mec-iS

src/linalg/basic/matrix.rs Show resolved Hide resolved
@Mec-iS
Copy link
Collaborator

Mec-iS commented Apr 5, 2023

merged into #257

@Mec-iS Mec-iS closed this Apr 5, 2023
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