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

Max/Min functions for matrices #78

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

malloc82
Copy link

Added functions for Matrix class that has similar behavior as in Matlab's max/min when applied to matrices:
http://www.mathworks.com/help/matlab/ref/max.html

similar behavior as in max/min functions in Matlab
@mikera
Copy link
Owner

mikera commented Jun 24, 2015

Hi @malloc82 , very happy to consider this, though I think the implementation still needs a bit of work

Specific comments:

  • If we want to implement these functions, I think they should implemented at the class AMatrix so that they work for all matrix types. Can be overriden for Matrix etc.
  • Shouldn't these return vectors rather than matrices?
  • You may find that getRow(i).elementMin() is an effective way of computing the row minimum. This has the advantage of working for any type of AMatrix, and avoids duplicating a lot of fiddly loop code
  • Please provide tests along with new functions like this

@malloc82
Copy link
Author

Hi Mike,
I really like your work, and I found myself using it more and more with Clojure. I think I might be able to contribute some here. Probably start from small stuffs. As to your comments:

  • I think the result of these functions should reflect the dimension of the data and query type, as in, rowMax should return a column vector, colMax should return a row vector. This is also the behavior of corresponding Matlab functions. From what I see so far there is no way to distinguish between a row vector or a column vector. So I decided to return a matrix type instead.
  • I agree the duplicated loop code is not too great, but instead of using getRow(i).elementMin() I'm thinking about either modify existing one or add additional function like this:
    public static final double elementMin(double[] data, int offset, int length, int step) {
        double result = data[offset];
        for (int i=1; i<length; i+=step) {
            double v=data[offset+i];
            if (v<result) result=v;
        }
        return result;
    }

in DoubleArray class. So basically passing a parameter to let i increment by step. This way it can be utilized in old code and as well as in those column max/min functions. For safety, we can just add them as new functions for now since there is an additional parameter, which do make it a new function. What do you think about this?

  • I'll make sure I implement this on AMatrix level with test cases as well.

@mikera
Copy link
Owner

mikera commented Jun 26, 2015

Well... in both Vectorz and core.matrix there isn't a distinction between "row vector" and "column vector". From a logical perspective this is analogous with 1D arrays in programming languages.

I've debated this with various experts and the consensus we have come to is that this is better than the approach used in MATLAB (which basically treats everything as a matrix, so your only option is to use "row vectors" and "column vectors").

There is some documentation / discussion on the topic here:

https://github.com/mikera/core.matrix/wiki/Vectors-vs.-matrices

@mikera
Copy link
Owner

mikera commented Jun 26, 2015

In theory I like the idea of an static elementMin with step function. It could be re-used in some other places as well, e.g. StridedVector and StridedMatrix

@mikera
Copy link
Owner

mikera commented Dec 28, 2015

@malloc82 Are you still working on this? Or should I close?

@malloc82
Copy link
Author

Ah, sorry, for the delay. I'll try to push the change in the next week or so, been quite busy almost forgot about it.

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.

3 participants