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

Generic QR factorization #9

Merged
merged 9 commits into from
May 16, 2014
Merged

Conversation

vbarrielle
Copy link
Contributor

Hi,

Please find here a proposed implementation of a generic QR factorization. It relies on extending the Indexable trait so that it is implemented by all data types, and implements the ColSlice and RowSlice traits for static sized matrices by returning a DVec. For now I can't find a better way to implement this trait.

Vincent Barrielle added 3 commits May 11, 2014 20:05
This allows the implementation of householder reflection without relying
on knowledge of DVec. This required a new member in the Indexable trait:
the shape() function, which returns the maximum index available.
But static matrices can't use it yet, they need to implement the
Row/Col slicing traits.
The ColSlice implementation for fixed size matrices returns a DVec,
while this is probably not optimal performance-wise, the dynamic nature
of the result makes this necessary. Using a data type presenting the
ImmutableVector trait would solve this, but it looks like a non-trivial
change.
let offset2 = self.offset(row2, col2);
let count = self.mij.len();
assert!(offset1 < count);
assert!(offset1 < count);
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated line. I guess this should be offset2 < count?

@sebcrozet
Copy link
Member

Hi, this is a very good work!
It would be great to add those two functions to the na.rs:

  • new_identity<M: Eye>(dim: uint) -> M { Eye::new_identity(dim) }
  • publicly re-export the linalg::decomposition::householder_matrix function.

This would allow the user to write na::new_identity(foo) and na::householder_matrix(foo) directly.

@vbarrielle
Copy link
Contributor Author

Hi,

I've updated my branch taking your remarks into account, and updated the code to the latest rust in the meanwhile. It is now required to implement the trait FloatMath in order to use math methods such as sin() or exp(). In the process of converting to FloatMath where necessary I've also removed some overspecified traits (like Num+Float)

sebcrozet added a commit that referenced this pull request May 16, 2014
@sebcrozet sebcrozet merged commit cfa1825 into dimforge:master May 16, 2014
@sebcrozet
Copy link
Member

Thanks!

@vbarrielle vbarrielle deleted the genericQR branch May 16, 2014 20:13
vihdzp added a commit to vihdzp/nalgebra that referenced this pull request Jul 17, 2021
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.

2 participants