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

iter::Sum for DMatrix #517

Closed
wants to merge 7 commits into from
Closed

Conversation

jswrenn
Copy link
Sponsor Contributor

@jswrenn jswrenn commented Dec 29, 2018

This is one potential way to address #514. It tackles the problem in three steps:

  1. The foundation of this approach is to introduce a notion of Default for dimensions. Statically known dimensions default to their value (this is already the behavior of the Default implementation provided by typenum). The Default of a Dynamic dimension is 0.
  2. Using this, we can then lift DimName requirements on Matrix constructors that required them; any dynamic dimensions simply default to zero size.
  3. Finally, we can lift the DimName requirements on functions that used these constructors, like the implementation of iter::Sum.

This PR only lifts the DimName requirement on iter::Sum, but the approach is broadly applicable and we can use it to lift DimName requirements in many parts of nalgebra.

Dynamic dimensions default to `0`. Static dimensions default to themselves.
Interpret dynamic dimensions as 0.
If the sequence is empty, a zero matrix is produced in which any dynamic dimensions default to size 0.

Fixes dimforge#514
@grtlr
Copy link
Contributor

grtlr commented Dec 30, 2018

That was a really quick response, thank you very much!

I have not found any test cases for the implementation of Sum in the current code base. If you want, I could write up some tests for the static and the dynamic case (as well as DVector)?

@jswrenn
Copy link
Sponsor Contributor Author

jswrenn commented Dec 30, 2018

@grtlr That would be great! In the interest of killing two birds with one stone, perhaps implement these as doctests on the sum functions?

If you fork and file a PR on my more-generic-sum branch, I'll merge them so they're included with this PR!

@grtlr
Copy link
Contributor

grtlr commented Jan 1, 2019

@jswrenn I have created a PR on your branch. If you have any comments regarding the formatting, feel free to let me know.

grtlr and others added 2 commits January 1, 2019 19:31
@vadixidav
Copy link
Contributor

Was literally just wanting this now. You solved it in the past. Awesome!

@sebcrozet
Copy link
Member

Thanks @jswrenn and @grtlr and very sorry for the late answer.

For a first implementation of this, we should just panic if the provided matrix iterator is empty. Indeed, this implementation of Zero for dynamic matrices does not really comply with what the Zero trait is intended to do. In particular, the returned matrix is not a valid zero element for any non-empty dynamically-sized matrix thus code like the following won't work as the user would expect:

let zero = DMatrix::zero();
let m = DMatrix::from_diagonal(2, 2, 1.0);
assert_eq!(m + zero, m)

@jswrenn jswrenn mentioned this pull request Feb 19, 2019
@jswrenn
Copy link
Sponsor Contributor Author

jswrenn commented Feb 19, 2019

I've opened a separate pull request implementing the panic-on-empty behavior: #552.

Sorry for the slow response; I've been recovering from some medical issues.

@jswrenn jswrenn closed this Feb 19, 2019
@sebcrozet
Copy link
Member

@jswrenn No worry. Take care!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants