Menu
▾
▴
pysparse-users
|
From: Daniel W. <dan...@gm...> - 2010-06-02 22:20:46
|
It seems that the find() method (lin 2986 in in Src/ll_mat.c) has a
memory leak. Calling PyDECREF on a_row, a_col and a_val fixes this
issue. I propose the following change:
$ svn diff
Index: Src/ll_mat.c
===================================================================
--- Src/ll_mat.c (revision 168)
+++ Src/ll_mat.c (working copy)
@@ -2987,6 +2987,7 @@
/* Convert an LL matrix into coordinate format */
+ PyObject *returnTuple;
PyArrayObject *a_row, *a_col, *a_val; /* Matrix in coordinate format */
npy_intp dmat[1]; /* Dimension descriptor */
int *pi, *pj; /* Intermediate pointers to
matrix data */
@@ -3014,10 +3015,16 @@
}
}
- return Py_BuildValue( "OOO",
- PyArray_Return( a_val ),
- PyArray_Return( a_row ),
- PyArray_Return( a_col ) );
+ returnTuple = Py_BuildValue( "OOO",
+ PyArray_Return( a_val ),
+ PyArray_Return( a_row ),
+ PyArray_Return( a_col ) );
+
+ Py_DECREF(a_row);
+ Py_DECREF(a_col);
+ Py_DECREF(a_val);
+
+ return returnTuple;
}
/*********************************/
If no one has any particular objection, I'll add that changes to to
branches/1_1_x and merge them to trunk/.
Cheers
--
Daniel Wheeler
|
|
From: <lu...@o2...> - 2010-06-02 23:41:26
|
Daniel Wheeler <dan...@gm...> writes:
> It seems that the find() method (lin 2986 in in Src/ll_mat.c) has a
> memory leak. Calling PyDECREF on a_row, a_col and a_val fixes this
> issue. I propose the following change:
I propose simpler (`N' is like `O' but without PyINCREF, details below):
$ git diff
diff --git a/Src/ll_mat.c b/Src/ll_mat.c
index 230829a..1adee2f 100644
--- a/Src/ll_mat.c
+++ b/Src/ll_mat.c
@@ -3014,7 +3014,7 @@ static PyObject *LLMat_Find( LLMatObject *self, PyObject *args ) {
}
}
- return Py_BuildValue( "OOO",
+ return Py_BuildValue( "NNN",
PyArray_Return( a_val ),
PyArray_Return( a_row ),
PyArray_Return( a_col ) );
from File: python2.5-api.info, Node: Parsing arguments and building values
``N' (object) {[PyObject * }]'
Same as `O', except it doesn't increment the reference count
on the object. Useful when the object is created by a call
to an object constructor in the argument list.
|
|
From: Dominique O. <dom...@gm...> - 2010-06-03 06:57:00
|
2010/6/3 Łukasz Pankowski <lu...@o2...>:
> Daniel Wheeler <dan...@gm...> writes:
>
>> It seems that the find() method (lin 2986 in in Src/ll_mat.c) has a
>> memory leak. Calling PyDECREF on a_row, a_col and a_val fixes this
>> issue. I propose the following change:
>
> I propose simpler (`N' is like `O' but without PyINCREF, details below):
>
> $ git diff
> diff --git a/Src/ll_mat.c b/Src/ll_mat.c
> index 230829a..1adee2f 100644
> --- a/Src/ll_mat.c
> +++ b/Src/ll_mat.c
> @@ -3014,7 +3014,7 @@ static PyObject *LLMat_Find( LLMatObject *self, PyObject *args ) {
> }
> }
>
> - return Py_BuildValue( "OOO",
> + return Py_BuildValue( "NNN",
> PyArray_Return( a_val ),
> PyArray_Return( a_row ),
> PyArray_Return( a_col ) );
>
>
> from File: python2.5-api.info, Node: Parsing arguments and building values
>
> ``N' (object) {[PyObject * }]'
> Same as `O', except it doesn't increment the reference count
> on the object. Useful when the object is created by a call
> to an object constructor in the argument list.
Thanks. I fixed it in the new-setup branch as well.
--
Dominique
|
|
From: Daniel W. <dan...@gm...> - 2010-06-03 19:41:00
|
Can I do a 1.1.1 release? Any objections? Find() method is quite
important for fipy right now.
On Thu, Jun 3, 2010 at 10:10 AM, Dominique Orban
<dom...@gm...> wrote:
> Yes please. Thanks for finding this bug!
>
> On Thu, Jun 3, 2010 at 3:47 PM, Daniel Wheeler
> <dan...@gm...> wrote:
>> Shall I do this on the version 1.1 branch and trunk?
>>
>> 2010/6/3 Dominique Orban <dom...@gm...>:
>>> 2010/6/3 Łukasz Pankowski <lu...@o2...>:
>>>> Daniel Wheeler <dan...@gm...> writes:
>>>>
>>>>> It seems that the find() method (lin 2986 in in Src/ll_mat.c) has a
>>>>> memory leak. Calling PyDECREF on a_row, a_col and a_val fixes this
>>>>> issue. I propose the following change:
>>>>
>>>> I propose simpler (`N' is like `O' but without PyINCREF, details below):
>>>>
>>>> $ git diff
>>>> diff --git a/Src/ll_mat.c b/Src/ll_mat.c
>>>> index 230829a..1adee2f 100644
>>>> --- a/Src/ll_mat.c
>>>> +++ b/Src/ll_mat.c
>>>> @@ -3014,7 +3014,7 @@ static PyObject *LLMat_Find( LLMatObject *self, PyObject *args ) {
>>>> }
>>>> }
>>>>
>>>> - return Py_BuildValue( "OOO",
>>>> + return Py_BuildValue( "NNN",
>>>> PyArray_Return( a_val ),
>>>> PyArray_Return( a_row ),
>>>> PyArray_Return( a_col ) );
>>>>
>>>>
>>>> from File: python2.5-api.info, Node: Parsing arguments and building values
>>>>
>>>> ``N' (object) {[PyObject * }]'
>>>> Same as `O', except it doesn't increment the reference count
>>>> on the object. Useful when the object is created by a call
>>>> to an object constructor in the argument list.
>>>
>>> Thanks. I fixed it in the new-setup branch as well.
>>>
>>> --
>>> Dominique
>>>
>>
>>
>>
>> --
>> Daniel Wheeler
>>
>
>
>
> --
> Dominique
>
--
Daniel Wheeler
|
|
From: <lu...@o2...> - 2010-06-03 22:20:15
|
Daniel Wheeler <dan...@gm...> writes: > Can I do a 1.1.1 release? Any objections? Find() method is quite > important for fipy right now. Could you apply my old patch (2009-12-09) before the release: ll_mat: fix raising IndexError for invalid indices - ID: 2911619 http://sourceforge.net/tracker/?func=detail&aid=2911619&group_id=101403&atid=629638 The patch has unclear description (better below) and probably will fail (another of my patches already applied is changing the same lines) -- I can update the patch if you are willing to apply it. Better description: >>> from pysparse.spmatrix import ll_mat >>> a = ll_mat(3,3) >>> a[0,0] 0.0 >>> print a[0] None >>> print a[0,0,0] None But "a[0]" and "a[0,0,0]" should raise IndexError and that is what happens in the C function, but the function returns None instead of NULL and that is why the exception is not actually raised. > > On Thu, Jun 3, 2010 at 10:10 AM, Dominique Orban > <dom...@gm...> wrote: >> Yes please. Thanks for finding this bug! >> >> On Thu, Jun 3, 2010 at 3:47 PM, Daniel Wheeler >> <dan...@gm...> wrote: >>> Shall I do this on the version 1.1 branch and trunk? >>> >>> 2010/6/3 Dominique Orban <dom...@gm...>: >>>> 2010/6/3 Łukasz Pankowski <lu...@o2...>: >>>>> Daniel Wheeler <dan...@gm...> writes: >>>>> >>>>>> It seems that the find() method (lin 2986 in in Src/ll_mat.c) has a >>>>>> memory leak. Calling PyDECREF on a_row, a_col and a_val fixes this >>>>>> issue. I propose the following change: >>>>> >>>>> I propose simpler (`N' is like `O' but without PyINCREF, details below): >>>>> >>>>> $ git diff >>>>> diff --git a/Src/ll_mat.c b/Src/ll_mat.c >>>>> index 230829a..1adee2f 100644 >>>>> --- a/Src/ll_mat.c >>>>> +++ b/Src/ll_mat.c >>>>> @@ -3014,7 +3014,7 @@ static PyObject *LLMat_Find( LLMatObject *self, PyObject *args ) { >>>>> } >>>>> } >>>>> >>>>> - return Py_BuildValue( "OOO", >>>>> + return Py_BuildValue( "NNN", >>>>> PyArray_Return( a_val ), >>>>> PyArray_Return( a_row ), >>>>> PyArray_Return( a_col ) ); >>>>> >>>>> >>>>> from File: python2.5-api.info, Node: Parsing arguments and building values >>>>> >>>>> ``N' (object) {[PyObject * }]' >>>>> Same as `O', except it doesn't increment the reference count >>>>> on the object. Useful when the object is created by a call >>>>> to an object constructor in the argument list. >>>> >>>> Thanks. I fixed it in the new-setup branch as well. >>>> >>>> -- >>>> Dominique >>>> >>> >>> >>> >>> -- >>> Daniel Wheeler >>> >> >> >> >> -- >> Dominique >> |
|
From: Dominique O. <dom...@gm...> - 2010-06-04 06:56:38
|
2010/6/4 Łukasz Pankowski <lu...@o2...>: > Daniel Wheeler <dan...@gm...> writes: > >> Can I do a 1.1.1 release? Any objections? Find() method is quite >> important for fipy right now. > > Could you apply my old patch (2009-12-09) before the release: > > ll_mat: fix raising IndexError for invalid indices - ID: 2911619 > http://sourceforge.net/tracker/?func=detail&aid=2911619&group_id=101403&atid=629638 Lukasz, Very sorry we missed this patch somehow. I fixed the new-setup branch. Daniel, should we be thinking about making new-setup the default in Pysparse? That is the one using the "standard" distutils layout and is the one in the Python Package Index. Have you tried it with FiPy? -- Dominique |
|
From: Daniel W. <dan...@gm...> - 2010-06-04 13:45:11
|
2010/6/4 Dominique Orban <dom...@gm...>: > Daniel, should we be thinking about making new-setup the default in > Pysparse? That is the one using the "standard" distutils layout and is > the one in the Python Package Index. Have you tried it with FiPy? Net yet. I'll try it with fipy and then you can merge it back into trunk. I'm going to make a release from the version 1_1_x branch anyway (not from trunk) with the minor bug fixes we've been making so this is irrelevant right now. Once you are happy with your merge we can do a version 1.2 release from trunk. Does that sound reasonable? -- Daniel Wheeler |
|
From: Dominique O. <dom...@gm...> - 2010-06-04 15:06:47
|
On Fri, Jun 4, 2010 at 3:37 PM, Daniel Wheeler <dan...@gm...> wrote: > 2010/6/4 Dominique Orban <dom...@gm...>: > >> Daniel, should we be thinking about making new-setup the default in >> Pysparse? That is the one using the "standard" distutils layout and is >> the one in the Python Package Index. Have you tried it with FiPy? > > Net yet. I'll try it with fipy and then you can merge it back into > trunk. I'm going to make a release from the version 1_1_x branch > anyway (not from trunk) with the minor bug fixes we've been making so > this is irrelevant right now. Once you are happy with your merge we > can do a version 1.2 release from trunk. Does that sound reasonable? Sounds good. I'll wait for your comments on using the new-setup branch with Fipy. There will be some namespace adjustments required. Of course, the namespace can be adjusted too. -- Dominique |
|
From: Daniel W. <dan...@gm...> - 2010-06-04 21:26:28
|
Dominque, You may run into quite a few conflicts when you try and merge your new-setup branch back to trunk. If the bug fixes are being duplicated you will have some reconciling to do. Ideally, you'd make the bug fix on trunk and then merge trunk to your new-setup branch keeping a record in the logs of the revision numbers. To merge new-setup back in to trunk you would first merge trunk to new-setup and reconcile and then merge back the other way. Anyway, we are quite careful about this type of thing in fipy, see <http://matforge.org/fipy/browser/trunk/documentation/ADMINISTRATA.txt>. We may want to implement a similar policy for pysparse or add this ADMINISTRATA file with suitable modifications. Cheers. On Fri, Jun 4, 2010 at 11:06 AM, Dominique Orban <dom...@gm...> wrote: > On Fri, Jun 4, 2010 at 3:37 PM, Daniel Wheeler > <dan...@gm...> wrote: >> 2010/6/4 Dominique Orban <dom...@gm...>: >> >>> Daniel, should we be thinking about making new-setup the default in >>> Pysparse? That is the one using the "standard" distutils layout and is >>> the one in the Python Package Index. Have you tried it with FiPy? >> >> Net yet. I'll try it with fipy and then you can merge it back into >> trunk. I'm going to make a release from the version 1_1_x branch >> anyway (not from trunk) with the minor bug fixes we've been making so >> this is irrelevant right now. Once you are happy with your merge we >> can do a version 1.2 release from trunk. Does that sound reasonable? > > Sounds good. I'll wait for your comments on using the new-setup branch > with Fipy. There will be some namespace adjustments required. Of > course, the namespace can be adjusted too. > > -- > Dominique > -- Daniel Wheeler |
|
From: Daniel W. <dan...@gm...> - 2010-06-04 21:50:03
|
One more thing when I build pysparse I noticed that I'm getting a "-g" compiler argument? For example, gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fPIC -DUSE_VENDOR_BLAS=1 -DNUMPY=1 -Isuperlu -IInclude -I/users/wd15/usr/i686/5.0/lib/python2.5/site-packages/numpy/core/include -I/usr/include/python2.5 -c superlu/util.c -o build/temp.linux-i686-2.5/superlu/util.o How do I get rid of that? I'm using the setup.py from 1_1_x. 2010/6/4 Dominique Orban <dom...@gm...>: > 2010/6/4 Łukasz Pankowski <lu...@o2...>: >> Daniel Wheeler <dan...@gm...> writes: >> >>> Can I do a 1.1.1 release? Any objections? Find() method is quite >>> important for fipy right now. >> >> Could you apply my old patch (2009-12-09) before the release: >> >> ll_mat: fix raising IndexError for invalid indices - ID: 2911619 >> http://sourceforge.net/tracker/?func=detail&aid=2911619&group_id=101403&atid=629638 > > Lukasz, > > Very sorry we missed this patch somehow. I fixed the new-setup branch. > > Daniel, should we be thinking about making new-setup the default in > Pysparse? That is the one using the "standard" distutils layout and is > the one in the Python Package Index. Have you tried it with FiPy? > > -- > Dominique > -- Daniel Wheeler |
|
From: Daniel W. <dan...@gm...> - 2010-06-04 22:02:31
|
Just added version 1.1.1 to sourceforge and created tags/version-1_1_1. On Fri, Jun 4, 2010 at 5:49 PM, Daniel Wheeler <dan...@gm...> wrote: > One more thing when I build pysparse I noticed that I'm getting a "-g" > compiler argument? For example, > > gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall > -Wstrict-prototypes -fPIC -DUSE_VENDOR_BLAS=1 -DNUMPY=1 -Isuperlu > -IInclude -I/users/wd15/usr/i686/5.0/lib/python2.5/site-packages/numpy/core/include > -I/usr/include/python2.5 -c superlu/util.c -o > build/temp.linux-i686-2.5/superlu/util.o > > How do I get rid of that? I'm using the setup.py from 1_1_x. > > 2010/6/4 Dominique Orban <dom...@gm...>: >> 2010/6/4 Łukasz Pankowski <lu...@o2...>: >>> Daniel Wheeler <dan...@gm...> writes: >>> >>>> Can I do a 1.1.1 release? Any objections? Find() method is quite >>>> important for fipy right now. >>> >>> Could you apply my old patch (2009-12-09) before the release: >>> >>> ll_mat: fix raising IndexError for invalid indices - ID: 2911619 >>> http://sourceforge.net/tracker/?func=detail&aid=2911619&group_id=101403&atid=629638 >> >> Lukasz, >> >> Very sorry we missed this patch somehow. I fixed the new-setup branch. >> >> Daniel, should we be thinking about making new-setup the default in >> Pysparse? That is the one using the "standard" distutils layout and is >> the one in the Python Package Index. Have you tried it with FiPy? >> >> -- >> Dominique >> > > > > -- > Daniel Wheeler > -- Daniel Wheeler |
|
From: Daniel W. <dan...@gm...> - 2010-06-07 17:16:07
|
Hi Dominique, I tried the new-setup-py branch against fipy and everything worked fine with the following small change: bunter[wd15]: svn diff Index: pysparse/__init__.py =================================================================== --- pysparse/__init__.py (revision 177) +++ pysparse/__init__.py (working copy) @@ -5,6 +5,7 @@ # Imports from numpy._import_tools import PackageLoader from version import version as __version__ +from sparse import spmatrix #from sparse import * from misc import get_include The organization seems greatly improved. Thanks for all the hard work. Is the change above acceptable? On Fri, Jun 4, 2010 at 11:06 AM, Dominique Orban <dom...@gm...> wrote: > On Fri, Jun 4, 2010 at 3:37 PM, Daniel Wheeler > <dan...@gm...> wrote: >> 2010/6/4 Dominique Orban <dom...@gm...>: >> >>> Daniel, should we be thinking about making new-setup the default in >>> Pysparse? That is the one using the "standard" distutils layout and is >>> the one in the Python Package Index. Have you tried it with FiPy? >> >> Net yet. I'll try it with fipy and then you can merge it back into >> trunk. I'm going to make a release from the version 1_1_x branch >> anyway (not from trunk) with the minor bug fixes we've been making so >> this is irrelevant right now. Once you are happy with your merge we >> can do a version 1.2 release from trunk. Does that sound reasonable? > > Sounds good. I'll wait for your comments on using the new-setup branch > with Fipy. There will be some namespace adjustments required. Of > course, the namespace can be adjusted too. > > -- > Dominique > -- Daniel Wheeler |
|
From: Dominique O. <dom...@gm...> - 2010-06-08 07:10:23
|
On Mon, Jun 7, 2010 at 7:15 PM, Daniel Wheeler <dan...@gm...> wrote: > Hi Dominique, > > I tried the new-setup-py branch against fipy and everything worked > fine with the following small change: > > bunter[wd15]: svn diff > Index: pysparse/__init__.py > =================================================================== > --- pysparse/__init__.py (revision 177) > +++ pysparse/__init__.py (working copy) > @@ -5,6 +5,7 @@ > # Imports > from numpy._import_tools import PackageLoader > from version import version as __version__ > +from sparse import spmatrix > > #from sparse import * > from misc import get_include > > The organization seems greatly improved. Thanks for all the hard work. > Is the change above acceptable? Hi Daniel, I don't see any problem with the above change. The branch is updated. Let's now make new-setup the default. Cheers, -- Dominique |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X