Path: news.mathworks.com!not-for-mail
From: <HIDDEN>
Newsgroups: comp.soft-sys.matlab
Subject: Re: help regarding 2D matrix using c mex file
Date: Wed, 19 Aug 2009 07:52:01 +0000 (UTC)
Organization: Boeing
Lines: 117
Message-ID: <h6gav1$hjm$1@fred.mathworks.com>
References: <h6b2k6$oh2$1@fred.mathworks.com> <h6b3nq$6ft$1@fred.mathworks.com> <h6dsve$37p$1@fred.mathworks.com> <h6et12$284$1@fred.mathworks.com> <h6fhbo$l34$1@fred.mathworks.com>
Reply-To: <HIDDEN>
NNTP-Posting-Host: webapp-03-blr.mathworks.com
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8bit
X-Trace: fred.mathworks.com 1250668321 18038 172.30.248.38 (19 Aug 2009 07:52:01 GMT)
X-Complaints-To: news@mathworks.com
NNTP-Posting-Date: Wed, 19 Aug 2009 07:52:01 +0000 (UTC)
X-Newsreader: MATLAB Central Newsreader 756104
Xref: news.mathworks.com comp.soft-sys.matlab:564370


"oruganti murthy" <omurthy@yahoo.com> wrote in message <h6fhbo$l34$1@fred.mathworks.com>...
>
> Here is the code and the output I've got.
> Please let me know if I can improve it any more

The code has some very serious errors. I am surprised you got anything reasonable for output. It corrupts memory and crashes my machine. Here are my comments:

> 
> *************
> #include "mex.h"
> #include <math.h>
> 
> #define OUT        plhs[0]
> #define SAMPLE double   /* define the type used for data samples */
> 
> /****************************************************************************/
> /* Function Declarations                                                    */
> /****************************************************************************/
> SAMPLE **get_matrix_pointers(int xDim, int yDim, SAMPLE *output);

Can't comment on this one since you didn't show the code for this function. But I strongly suspect that it is incorrect based on your SAMPLE ** return type. See discussion below.

> void release_matrix_pointers(SAMPLE **a);

Get rid of this ... see comments below.

> void py_dist(SAMPLE **a, int xDim, int yDim);

This prototype doesn't match your function. The prototype has only three arguments, while your function definition has four. You would have seen a compiler warning had you used the -v option with the mex command. Also, based on your intended usage there are too many levels of indirection for "a".

> /****************************************************************************/
> /* Gateway function and error checking                                      */
> /****************************************************************************/
> void mexFunction(int nlhs, mxArray* plhs[], int nrhs, const mxArray* prhs[])
> {

Note that the prhs points to a const mxArray ... i.e. it is intended to be read-only.

>    SAMPLE **output, **a;

No, this is bad. These are being used as pointers to data areas, so there should only be a single level of indirection. i.e., the mxGetPr returns a double *, not a double **. So you should have this instead:

SAMPLE *output, *a;

>    int xDim, yDim;
>    
>    xDim = (int) mxGetM(prhs[0]);
>    yDim = (int) mxGetN(prhs[0]);

Moot point for most inputs, but be advised that later versions of MATLAB the return type of mxGetM and mxGetN is not int but mwSize, which may be an unsigned int. So if you happen to get a very large matrix as input that has a size larger than the largest int, the result of these lines will overflow the int and produce implementation dependent results. On 2's complement machines, that usually means modulo result, meaning the overflow will wrap around to negative values and you will end up with xDim and yDim being negative. Better to stick with the return type of these functions. e.g.,

    mwSize xDim, yDim;
    xDim = mxGetM(prhs[0]);
    yDim = mxGetN(prhs[0]);

>    /*do the matlab stuff*/
>    mexPrintf("x Dimensions = %d.\n",xDim);
>    mexPrintf("y Dimensions = %d.\n",yDim);

And if you make the change to mwSize above, then use an unsigned int format here. e.g., %u instead of %d.

>    OUT = mxCreateDoubleMatrix(yDim, xDim, mxREAL); // mxArray is transpose of c matrix

I think you missed the point about the transpose stuff. In MATLAB, memory for a 2D array is stored in column order, whereas in C memory for a 2D matrix is stored in row order. So if you take a 2x3 matrix from MATLAB and do a block memory copy into a matrix in C and want to access the elements in C using the [ ][ ] notation, you need to treat the C variable as a 3x2 matrix that is the transpose of the MATLAB matrix. My advice is to not use the [ ][ ] notation. Just use a double * and do your own indexing calculations to match the MATLAB column ordering of the data. It is easier to keep things straight in your code this way, and avoids the extra code needed to set up the [ ][ ] access syntax. So no need to switch xDim and yDim.

>    output = mxGetPr(OUT);
>    a = mxGetPr(prhs[0]);  
>    py_dist(a, xDim, yDim, output);

More arguments than prototype as mentioned above.

>    release_matrix_pointers(a);

This is very bad. You should *never* free anything associated with an input mxArray such as prhs[0]. In this case, you are passing the result of the a = mxGetPr(prhs[0]) to your routine and then subsequently calling free(a+1). First, a+1 is an invalid address to pass back to free so the result of the free(a+1) call is undefined. Not sure what you intended here. Maybe nothing bad will happen, or maybe corrupt memory and program crash. 2nd, as pointed out above, you shouldn't be doing this anyway since "a" is associated with a const (i.e., intended to be read-only) input. Get rid of this line.

/****************************************************************************/
> /* Free the space when we're done (just the array pointers)                 */
> /****************************************************************************/
> void release_matrix_pointers(SAMPLE **a)
> {
>     mxFree(a+1);
> } 

Get rid of this routine.

/****************************************************************************/
> /* Fill with pythagorian distances with the nice offset pointers            */
> /****************************************************************************/
> void py_dist(SAMPLE **a, int xDim, int yDim, SAMPLE **output)
> {
>     int x,y;
>     for(x = 0; x<=xDim; ++x)
>         for (y = 0; y<=yDim; ++y)
>             *output++ = *a++ ;
> }

This is amazing, at least to me, that it worked as well as it did. You are passing in a (double *), recast as a (double **). Then this line does a single dereference and copy:

>             *output++ = *a++ ;

The result of the *output operation is a (double *) type. So you are doing this type assignment:

    (double *) = (double *);

i.e., you are assigning a double * to another double *, even though the underlying data is double, not double *. So it seems to me that an incorrect number of bytes is being copied, and as an incorrect type to boot. Here is what you should be doing (getting rid of one level of indirection on "a" and "output"):

 void py_dist(SAMPLE *a, int xDim, int yDim, SAMPLE *output)
 {
     int x,y;
     for(x = 0; x<=xDim; ++x)
         for (y = 0; y<=yDim; ++y)
             *output++ = *a++ ;
 }

Finally, if you are just doing an element-by-element calculation, it is easier to use mxGetNumberOfElements and use a single for loop to access all the elements linearly.

James Tursa