Added basic insert method to add a matrix inside another matrix.

[sylvia43]

I'm still working on this. Nowhere near done. We can move the conversation from #193 here.

[sylvia43]

So why are rows and columns fields? Is it because they're not supposed to change and stick with the object?

[SamoylovMD]

Uh, hope you know what you do. We need additional version of this feature for compressed storages to avoid performance troubles (note that get() and other simple operations work more than O(1) on compressed matrices).

Also, I think some tests are required for this feature (part of #173).

[sylvia43]

Yup definately. Nowhere near done. I'm just trying to get more feedback and direction as I work on it.

[vkostyukov]

@SamoylovMD, for now we don't need an additional version for compressed entries. I will revisit it once I finish operations. Having just a common implementation for now is just fine.

[vkostyukov]

Overall, this looks very nice. Just a minor concern about params naming. Also, please don't forget to alter all the methods you add with Factory param. I know this annoying, but we have to follow this style:

Matrix fancyMethod(int i, int j) {
  return fancyMethod(int i, int j, factory /* default factory of this matrix */);
}
Matrix fancyMethod(int i, int j, Factory factory) {
  // now you know how to create output matrix
}

[SamoylovMD]

@vkostyukov What is operations stuff?

[vkostyukov]

I started doing this for vectors. Here is the example of inner product for vectors: https://github.com/vkostyukov/la4j/blob/master/src/main/java/org/la4j/vector/operation/ooplace/OoPlaceInnerProduct.java. The idea is to have efficient implementations of the algorithms based on vector/matrix type. So we have all the possible combinations in a single operation and then double dispatch there our arguments. See pipeTo method.

[vkostyukov]

So, I plan to continue migrating current implementations from AbstractVector/AbstractMatrix to operations.

[SamoylovMD]

Hm. So, is it a simplification? For example, if I want to add vector to another one, what will I have to do exactly? Or is it just an additional feature?

[vkostyukov]

This is a performance improvement. The API is unchanged. In case of adding vectors, you will write something like this:

Vector a  = new DenseVector();
Vector b = new SparseVector();
Vector c = a.add(b); // will chose the best implementation of "add" operation based on vectors types;

Users will not notify the change. But the performance will be improved dramatically. See source code of CompressedVector.

[vkostyukov]

@SamoylovMD, here is what I was talking about: https://github.com/vkostyukov/la4j/blob/master/src/main/java/org/la4j/vector/AbstractVector.java#L120. The pipeTo approach is something like double dispatch OO pattern + currying from functional programming.

[sylvia43]

I can't name it rows, that would hide the class field of the same name.

[vkostyukov]

You surely can. Just use this.rows and this.columns to reference the object fields.

[sylvia43]

But that looks ugly :disappointed:

[vkostyukov]

Yeah, I don't like mentioning this either, but I believe we have to follow the style we have. Or we can try to refactor all of those methods to avoid using conflicting names.

[sylvia43]

Does that look good? I'm taking a look at test cases right now, need to add some for this. And a sparse vector implementation.

[vkostyukov]

Looks good! Didn't really get about the sparse vector implementation. What do you mean?

[sylvia43]

Never mind, I was thinking of something else. Anything else other than test cases?

[SamoylovMD]

What do you think about vector-to-matrix and vector-to-vector insertions?

[sylvia43]

I think one of them already exists, not sure. We should totally add it though.

[vkostyukov]

We do have vector to matrix insertion via setRow and setColumn. Vector to vector insertion might be implemented as part of this PR.

[sylvia43]

Bugs of the day:

https://github.com/vkostyukov/la4j/blob/master/src/main/java/org/la4j/matrix/AbstractMatrix.java#L228 https://github.com/vkostyukov/la4j/blob/master/src/main/java/org/la4j/matrix/AbstractMatrix.java#L238

[sylvia43]

Fixed a bunch of bugs and added some test cases.

I just did a quick rebase (on #225) to be up to date and avoid merge conflicts.

I'm still not done yet...

[vkostyukov]

Wow! What a nice catch! Awesome job @anubiann00b!

[vkostyukov]

Feel free to implement similar methods that insert vector into vector.

[sylvia43]

Ok I'll do that seperately.