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

mat: add Doer interface and implementations #126

Merged
merged 1 commit into from Jul 27, 2017
Merged

mat: add Doer interface and implementations #126

merged 1 commit into from Jul 27, 2017

Conversation

kortschak
Copy link
Member

@kortschak kortschak commented Jul 8, 2017

This is a proposal for an approach to optimising sparse or sparsish matrix accesses raised in #124 (comment)

Only 2eb65a2 is relevant to this PR and I'll revise the history of the PR when #95 is merged.

Depends #95.

@kortschak
Copy link
Member Author

Cleaned up and added SymBandDense implementation.

PTAL

@btracey
Copy link
Member

btracey commented Jul 20, 2017

Can we find a better name than Do? In particular, the key feature of this is that it only operates on the "non-provably zero" region. The name should reflect that somehow.

@kortschak
Copy link
Member Author

Please suggest a name. I have nothing better.

@btracey
Copy link
Member

btracey commented Jul 25, 2017

How about WalkNZ? It iterates over the (provably) non-zero entries. Or, ReduceNZ, because reduce will frequently be the intended purpose?

@kortschak
Copy link
Member Author

WalkNZ sounds like an adventure tourism campaign. I don't like either of walk or reduce because of loading from other domains (walk is a graph theoretic thing and reduce sounds like it comes from mapreduce). How about DoNonZero, DoRowNonZero and DoColNonZero?

@btracey
Copy link
Member

btracey commented Jul 25, 2017

The problem with Do is it means nothing. The method iterates over the non-zero entries and calls the function. CallNonZero? IterNonZero?

@kortschak
Copy link
Member Author

I'd argue that Do and Call are synonyms when the parameter is a func.

@btracey
Copy link
Member

btracey commented Jul 26, 2017

Okay. SGTM

@kortschak
Copy link
Member Author

PTAL

@kortschak
Copy link
Member Author

I've used "provably non-zero" instead of "filled". I don't like it much, so if you have a better suggestion, I'd happily change it.

@btracey
Copy link
Member

btracey commented Jul 26, 2017

It should be "not provably zero". We don't have any elements that are provably non-zero

@btracey
Copy link
Member

btracey commented Jul 26, 2017

I wonder if it's better that this call the function at the non-zero elements, rather than at the elements which are provably non-zero. First of all, the behavior is easier to understand. Second of all, it means this behavior works correctly for sparse matrices where often no elements are provably non-zero, but many elements will actually be non-zero.

@btracey
Copy link
Member

btracey commented Jul 26, 2017

(and the change improves the important sparsity properties where a bunch of elements are known to be zero, so they don't have to be checked)

@btracey
Copy link
Member

btracey commented Jul 26, 2017

(also the method name doesn't have to be changed :) )

@kortschak
Copy link
Member Author

I'm happy to call it "non-zero element", except that a band created by mat.NewBand(3, 3, 0, 0, make([]float64, 3)) is all zero, but the function will be called with {0, 0, 0}, {1, 1, 0} and {2, 2, 0}. This is why I used "filled" before.

@btracey
Copy link
Member

btracey commented Jul 26, 2017

I understand that is the behavior now, but I'm suggesting the function is more generally useful if it doesn't call the function at all for zero elements. Meaning, for mat.NewBand(3, 3, 0, 0, make([]float64, 3)), the function isn't called at all. This behavior is much more understandable from an interface perspective.

Code that relies on the "provably" part will have to do a type switch anyway, because you can't tell what "provably" means from the interface. In that light, I think it's better to not call at all for zero entries.

@kortschak
Copy link
Member Author

Code that relies on the "provably" part will have to do a type switch anyway, because you can't tell what "provably" means from the interface. In that light, I think it's better to not call at all for non-zero entries.

OK. I can't do that today. I'll get back to this later.

@kortschak
Copy link
Member Author

kortschak commented Jul 26, 2017

Was less than I thought - the tests were already written with only non-zero elements in the bands.

btracey
btracey previously approved these changes Jul 26, 2017

var got float64
fn := func(i, j int, v float64) {
got += v
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment to the effect of "this sets to the same value to test accessing"? I originally was thinking this was mutating the matrix to have new values for the subsequent tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

t.Errorf("unexpected Doer sum: got:%f want:%f", got, want)
}

got = 0
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a comment like "test that the sum over all the rows equals the sum over the matrix"? I was caught up a while on the statefulness of got.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kortschak
Copy link
Member Author

PTAL

Also clean up some documentation and missing type checks related to
tests for NonZeroDoers.
@kortschak kortschak merged commit ffd939f into master Jul 27, 2017
@kortschak kortschak deleted the doer branch July 27, 2017 01:15
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.

None yet

2 participants