stat/spatial: use mat.RowNonZeroDoer #124
Conversation
stat/spatial/spatial.go
Outdated
| sw += w | ||
| dwd += w * v | ||
| dww += w * w | ||
| if band, ok := locality.(mat.Banded); ok { |
There was a problem hiding this comment.
Sorry, I don't have time to do a full review at the moment, but can we abstract this somehow with an interface? Something like "non-zero apply"? I think we'd like to discourage this kind of type switching outside the matrix package, and instead should create good interfaces to allow higher packages to be agnostic.
There was a problem hiding this comment.
I'd rather not here; the primary non-dense matrix being used for this case will be band, so it seems unnecessary to switch on potentially all mat types.
It is probably worth adding an apply like that though for quick uses (do you have a name suggestion?)
There was a problem hiding this comment.
Are you sure? It seems reasonable that one would want to use this on some kind of taurus topology, where the matrix would be banded, except with some data on the corners. This would be really easy to implement with a banded matrix plus some extra storage, but would not be compatible with this type switch.
There was a problem hiding this comment.
(actually, interestingly, that taurus structure could be done with the blas.Banded layout except actually using the * locations)
There was a problem hiding this comment.
Yes, I have thought that that might be useful. However, I don't see a way to define an arbitrary set of non-zero elements that reduces from O(n^2) time (space can be trivially - a type wrapping *mat.BandDense for example that is not a mat.Banded and uses those * elements).
We should make an issue to design/implement the function you're suggesting, but I don't think it will capture all cases without being entirely ad hoc, or unwieldy for the user to implement the interface.
There was a problem hiding this comment.
I have a solution. If we reduce the Apply method from its current signature to Apply(func(i, j int, v float64)), then an Applier interface can be used. This makes the Apply method slightly less efficient for its current use since a would need to be referred to in a closure, but I think the trade-off is probably worth it. Then we just add Apply to each of our types and note in the documentation that it only applies to the filled region of the receiver. Alternatively, a new Do with the signature above could be invented.
There was a problem hiding this comment.
I've tried this, and it's a little more complicated than I thought it would be. You really need a Do, a DoRow and a DoCol, or a way to restrict to columns/rows. The Do method works find for GetisOrd, though it needs to return without work for m-1 rows.
|
PTAL |
kortschak
changed the title
@btracey Please take a look.
Depends #126 if using second approach.