I really like "const" usage, but isn't this too much?
Done, see below
[dalcinl at botafogo petsc-dev]$ hg log -r 13064
changeset: 13064:733a706c5f5b
user:Lisandro Dalcin
date:Wed Dec 03 20:00:58 2008 -0300
summary: smarter VecPlaceArray/VecResetArray
[dalcinl at botafogo petsc-dev]$ hg backout -r 13064
reverting src/vec/vec/impls/mpi/pbvec.c
reverting src/vec/vec/impls/seq/dvec2.c
created new head
changeset 13068:a00a16279084 backs out changeset 13064:733a706c5f5b
the backout changeset is a new head - do not forget to merge
(use "backout --merge" if you want to auto-merge)
[dalcinl at botafogo petsc-dev]$ hg merge
2 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit)
[dalcinl at botafogo petsc-dev]$ hg commit -m 'merge'
On Wed, Dec 3, 2008 at 10:43 PM, Barry Smith wrote:
>
> Sorry, I don't know how to revert something that has been pushed.
> Can someone do it who has a brain?
>
> Barry
>
> On Dec 3, 2008, at 7:41 PM, Lisandro Dalcin wrote:
>
>> Barry, please revert the commit, I'll try to re-review this tomorrow.
>> I'm goin to bed.
>>
>> On Wed, Dec 3, 2008 at 10:40 PM, Lisandro Dalcin
>> wrote:
>>>
>>> Barry, are you talking about Vec's of local size zero? Damn me! I
>>> completelly forgot this case... But then, will not it be better to
>>> check and special case zero local sizes ??
>>>
>>> On Wed, Dec 3, 2008 at 10:37 PM, Lisandro Dalcin
>>> wrote:
On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith wrote:
>
> That is wierd; it seems strange that it would even compile.
OK, I'll try to review all this tomorrow, there are some other weird
Vec calls with such "const".
> BTW: I think your new change
> 7 - v->unplacedarray = v->array; /* save previous array so reset can
> bring
> it back */ 8
> - v->array = (PetscScalar *)a; 9 +
> if (a) { 10 + v->unplacedarray = v->array; /* save previous array so
> reset
> can bring it back */ 11
> + v->array = (PetscScalar *)a; 12 + }
> is TOTALLY wrong.
You see, I should never push before commenting :-(
> It is perfectly legitimate to place null arrays and one
> wants
> them to behave just like regular place arrays (saving the previous
> array).
What's the use case? This is used internally in PETSc? I do not
remember...
> Why did you make this change and what was wrong with the way it was?
Do you mean that it is legitimate taht if a user makes the nasty bug
of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/she
will get a nasty segfault when calling Vec operations (let say,
VecMax()) ???
Barry, feel free to revert the changeset. Anyway, please let me know
the rationale for this "feature", I still do not get it...
>
> Barry
>
>
> On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
>
>> VecPlaceArray(Vec vec,const PetscScalar array[])
>> VecReplaceArray(Vec vec,const PetscScalar array[])
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
>>>
>>>
>>>
>>> --
>>> Lisandro Dalc?n
>>> ---
>>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>>> Tel/Fax: +54-(0)342-451.1594
>>>
>>
>>
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith wrote:
>
> That is wierd; it seems strange that it would even compile.
OK, I'll try to review all this tomorrow, there are some other weird
Vec calls with such "const".
>BTW: I think your new change
> 7 - v->unplacedarray = v->array; /* save previous array so reset can bring
> it back */ 8
> - v->array = (PetscScalar *)a; 9 +
> if (a) { 10 + v->unplacedarray = v->array; /* save previous array so reset
> can bring it back */ 11
> + v->array = (PetscScalar *)a; 12 + }
> is TOTALLY wrong.
You see, I should never push before commenting :-(
> It is perfectly legitimate to place null arrays and one
> wants
> them to behave just like regular place arrays (saving the previous array).
What's the use case? This is used internally in PETSc? I do not remember...
> Why did you make this change and what was wrong with the way it was?
Do you mean that it is legitimate taht if a user makes the nasty bug
of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/she
will get a nasty segfault when calling Vec operations (let say,
VecMax()) ???
Barry, feel free to revert the commit
>
> Barry
>
>
> On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
>
>> VecPlaceArray(Vec vec,const PetscScalar array[])
>> VecReplaceArray(Vec vec,const PetscScalar array[])
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
fixes for VecPlaceArray/VecResetArray
Yes, but those VecCreateXXXWithArray() and null arrays are intended for create stub vecs for something like creating a VecScatter(). If PETSc creates such vector, it sould never return, or expose them to users, they do not have allocated space. I cannot see a use case of placing a NULL array. The vector is left in a useless state, and the allocated memory is still there. I believe that if one wants to clear the array, we should use VecReplaceArray(). This a different beast. Anyway, if this is just a difference in opinions or low-level details I?m missing, please just backout the changeset. On Wed, Dec 3, 2008 at 9:28 PM, Barry Smith wrote: > > One can call VecCreateXXXWithArray() with a null pointer for the array so > that > means there is already an established history of valid Vecs with null > arrays. > I'd like you do undo the changes you made unless you have a specific reason > for making the change (besides the reason of not having a Vec with a null > array). > > Barry > > On Dec 3, 2008, at 5:37 PM, Lisandro Dalcin wrote: > >> On Wed, Dec 3, 2008 at 8:08 PM, Matthew Knepley wrote: >>> >>> On Wed, Dec 3, 2008 at 5:04 PM, Lisandro Dalcin >>> wrote: I've pushed some fixes for VecPlaceArray/VecResetArray, trying to the pair a bit more smarter. However, the I'm not sure what's the use case in VecPlaceArray for accepting NULL for the array. Just in case, I did not changed this, but if NULL is actually pased, then the call is now non-op. Is this fine? I'm missing something?? >>> >>> I think you would like to reset the array back to NULL sometimes, to >>> clear >>> out >>> memory of an old array it no longer owns. Is this right? >>> >> >> Not sure if I understood you comment, but the companion call >> VecResetArray() restores the original array. Before my fix, the Vec >> was left actually pointing to a NULL. >> >> >> >>> -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594 >>> >>> -- >>> What most experimenters take for granted before they begin their >>> experiments >>> is infinitely more interesting than any results to which their >>> experiments >>> lead. >>> -- Norbert Wiener >>> >> >> >> >> -- >> Lisandro Dalc?n >> --- >> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) >> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) >> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >> Tel/Fax: +54-(0)342-451.1594 >> > > -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
Barry, please revert the commit, I'll try to re-review this tomorrow.
I'm goin to bed.
On Wed, Dec 3, 2008 at 10:40 PM, Lisandro Dalcin wrote:
> Barry, are you talking about Vec's of local size zero? Damn me! I
> completelly forgot this case... But then, will not it be better to
> check and special case zero local sizes ??
>
> On Wed, Dec 3, 2008 at 10:37 PM, Lisandro Dalcin wrote:
>> On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith wrote:
>>>
>>> That is wierd; it seems strange that it would even compile.
>>
>> OK, I'll try to review all this tomorrow, there are some other weird
>> Vec calls with such "const".
>>
>>>BTW: I think your new change
>>> 7 - v->unplacedarray = v->array; /* save previous array so reset can bring
>>> it back */ 8
>>> - v->array = (PetscScalar *)a; 9 +
>>> if (a) { 10 + v->unplacedarray = v->array; /* save previous array so reset
>>> can bring it back */ 11
>>> + v->array = (PetscScalar *)a; 12 + }
>>> is TOTALLY wrong.
>>
>> You see, I should never push before commenting :-(
>>
>>> It is perfectly legitimate to place null arrays and one
>>> wants
>>> them to behave just like regular place arrays (saving the previous array).
>>
>> What's the use case? This is used internally in PETSc? I do not remember...
>>
>>> Why did you make this change and what was wrong with the way it was?
>>
>> Do you mean that it is legitimate taht if a user makes the nasty bug
>> of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/she
>> will get a nasty segfault when calling Vec operations (let say,
>> VecMax()) ???
>>
>> Barry, feel free to revert the changeset. Anyway, please let me know
>> the rationale for this "feature", I still do not get it...
>>
>>
>>>
>>> Barry
>>>
>>>
>>> On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
>>>
VecPlaceArray(Vec vec,const PetscScalar array[])
VecReplaceArray(Vec vec,const PetscScalar array[])
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
>>>
>>>
>>
>>
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
Barry, are you talking about Vec's of local size zero? Damn me! I
completelly forgot this case... But then, will not it be better to
check and special case zero local sizes ??
On Wed, Dec 3, 2008 at 10:37 PM, Lisandro Dalcin wrote:
> On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith wrote:
>>
>> That is wierd; it seems strange that it would even compile.
>
> OK, I'll try to review all this tomorrow, there are some other weird
> Vec calls with such "const".
>
>>BTW: I think your new change
>> 7 - v->unplacedarray = v->array; /* save previous array so reset can bring
>> it back */ 8
>> - v->array = (PetscScalar *)a; 9 +
>> if (a) { 10 + v->unplacedarray = v->array; /* save previous array so reset
>> can bring it back */ 11
>> + v->array = (PetscScalar *)a; 12 + }
>> is TOTALLY wrong.
>
> You see, I should never push before commenting :-(
>
>> It is perfectly legitimate to place null arrays and one
>> wants
>> them to behave just like regular place arrays (saving the previous array).
>
> What's the use case? This is used internally in PETSc? I do not remember...
>
>> Why did you make this change and what was wrong with the way it was?
>
> Do you mean that it is legitimate taht if a user makes the nasty bug
> of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/she
> will get a nasty segfault when calling Vec operations (let say,
> VecMax()) ???
>
> Barry, feel free to revert the changeset. Anyway, please let me know
> the rationale for this "feature", I still do not get it...
>
>
>>
>> Barry
>>
>>
>> On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
>>
>>> VecPlaceArray(Vec vec,const PetscScalar array[])
>>> VecReplaceArray(Vec vec,const PetscScalar array[])
>>>
>>> --
>>> Lisandro Dalc?n
>>> ---
>>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>>> Tel/Fax: +54-(0)342-451.1594
>>>
>>
>>
>
>
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith wrote:
>
> That is wierd; it seems strange that it would even compile.
OK, I'll try to review all this tomorrow, there are some other weird
Vec calls with such "const".
>BTW: I think your new change
> 7 - v->unplacedarray = v->array; /* save previous array so reset can bring
> it back */ 8
> - v->array = (PetscScalar *)a; 9 +
> if (a) { 10 + v->unplacedarray = v->array; /* save previous array so reset
> can bring it back */ 11
> + v->array = (PetscScalar *)a; 12 + }
> is TOTALLY wrong.
You see, I should never push before commenting :-(
> It is perfectly legitimate to place null arrays and one
> wants
> them to behave just like regular place arrays (saving the previous array).
What's the use case? This is used internally in PETSc? I do not remember...
> Why did you make this change and what was wrong with the way it was?
Do you mean that it is legitimate taht if a user makes the nasty bug
of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/she
will get a nasty segfault when calling Vec operations (let say,
VecMax()) ???
Barry, feel free to revert the changeset. Anyway, please let me know
the rationale for this "feature", I still do not get it...
>
> Barry
>
>
> On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
>
>> VecPlaceArray(Vec vec,const PetscScalar array[])
>> VecReplaceArray(Vec vec,const PetscScalar array[])
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
fixes for VecPlaceArray/VecResetArray
On Wed, Dec 3, 2008 at 8:08 PM, Matthew Knepley wrote: > On Wed, Dec 3, 2008 at 5:04 PM, Lisandro Dalcin wrote: >> >> I've pushed some fixes for VecPlaceArray/VecResetArray, trying to the >> pair a bit more smarter. However, the I'm not sure what's the use case >> in VecPlaceArray for accepting NULL for the array. Just in case, I did >> not changed this, but if NULL is actually pased, then the call is now >> non-op. Is this fine? I'm missing something?? > > I think you would like to reset the array back to NULL sometimes, to clear > out > memory of an old array it no longer owns. Is this right? > Not sure if I understood you comment, but the companion call VecResetArray() restores the original array. Before my fix, the Vec was left actually pointing to a NULL. > >> >> -- >> Lisandro Dalc?n >> --- >> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) >> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) >> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >> Tel/Fax: +54-(0)342-451.1594 > > -- > What most experimenters take for granted before they begin their experiments > is infinitely more interesting than any results to which their experiments > lead. > -- Norbert Wiener > -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
fixes for VecPlaceArray/VecResetArray
I've pushed some fixes for VecPlaceArray/VecResetArray, trying to the pair a bit more smarter. However, the I'm not sure what's the use case in VecPlaceArray for accepting NULL for the array. Just in case, I did not changed this, but if NULL is actually pased, then the call is now non-op. Is this fine? I'm missing something?? -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
VecPlaceArray(Vec vec,const PetscScalar array[]) VecReplaceArray(Vec vec,const PetscScalar array[]) -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
I really like "const" usage, but isn't this too much?
Sorry, I don't know how to revert something that has been pushed.
Can someone do it who has a brain?
Barry
On Dec 3, 2008, at 7:41 PM, Lisandro Dalcin wrote:
> Barry, please revert the commit, I'll try to re-review this tomorrow.
> I'm goin to bed.
>
> On Wed, Dec 3, 2008 at 10:40 PM, Lisandro Dalcin
> wrote:
>> Barry, are you talking about Vec's of local size zero? Damn me! I
>> completelly forgot this case... But then, will not it be better to
>> check and special case zero local sizes ??
>>
>> On Wed, Dec 3, 2008 at 10:37 PM, Lisandro Dalcin
>> wrote:
>>> On Wed, Dec 3, 2008 at 9:17 PM, Barry Smith
>>> wrote:
That is wierd; it seems strange that it would even compile.
>>>
>>> OK, I'll try to review all this tomorrow, there are some other weird
>>> Vec calls with such "const".
>>>
BTW: I think your new change
7 - v->unplacedarray = v->array; /* save previous array so reset
can bring
it back */ 8
- v->array = (PetscScalar *)a; 9 +
if (a) { 10 + v->unplacedarray = v->array; /* save previous array
so reset
can bring it back */ 11
+ v->array = (PetscScalar *)a; 12 + }
is TOTALLY wrong.
>>>
>>> You see, I should never push before commenting :-(
>>>
It is perfectly legitimate to place null arrays and one
wants
them to behave just like regular place arrays (saving the
previous array).
>>>
>>> What's the use case? This is used internally in PETSc? I do not
>>> remember...
>>>
Why did you make this change and what was wrong with the way it
was?
>>>
>>> Do you mean that it is legitimate taht if a user makes the nasty bug
>>> of calling VecPlaceArray(x,ptr) with ptr being NULL, then next he/
>>> she
>>> will get a nasty segfault when calling Vec operations (let say,
>>> VecMax()) ???
>>>
>>> Barry, feel free to revert the changeset. Anyway, please let me know
>>> the rationale for this "feature", I still do not get it...
>>>
>>>
Barry
On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
> VecPlaceArray(Vec vec,const PetscScalar array[])
> VecReplaceArray(Vec vec,const PetscScalar array[])
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a
> (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica
> (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas
> (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
>>>
>>>
>>>
>>> --
>>> Lisandro Dalc?n
>>> ---
>>> Centro Internacional de M?todos Computacionales en Ingenier?a
>>> (CIMEC)
>>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica
>>> (INTEC)
>>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>>> Tel/Fax: +54-(0)342-451.1594
>>>
>>
>>
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
fixes for VecPlaceArray/VecResetArray
One can call VecCreateXXXWithArray() with a null pointer for the array so that means there is already an established history of valid Vecs with null arrays. I'd like you do undo the changes you made unless you have a specific reason for making the change (besides the reason of not having a Vec with a null array). Barry On Dec 3, 2008, at 5:37 PM, Lisandro Dalcin wrote: > On Wed, Dec 3, 2008 at 8:08 PM, Matthew Knepley > wrote: >> On Wed, Dec 3, 2008 at 5:04 PM, Lisandro Dalcin >> wrote: >>> >>> I've pushed some fixes for VecPlaceArray/VecResetArray, trying to >>> the >>> pair a bit more smarter. However, the I'm not sure what's the use >>> case >>> in VecPlaceArray for accepting NULL for the array. Just in case, I >>> did >>> not changed this, but if NULL is actually pased, then the call is >>> now >>> non-op. Is this fine? I'm missing something?? >> >> I think you would like to reset the array back to NULL sometimes, >> to clear >> out >> memory of an old array it no longer owns. Is this right? >> > > Not sure if I understood you comment, but the companion call > VecResetArray() restores the original array. Before my fix, the Vec > was left actually pointing to a NULL. > > > >> >>> >>> -- >>> Lisandro Dalc?n >>> --- >>> Centro Internacional de M?todos Computacionales en Ingenier?a >>> (CIMEC) >>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica >>> (INTEC) >>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >>> Tel/Fax: +54-(0)342-451.1594 >> >> -- >> What most experimenters take for granted before they begin their >> experiments >> is infinitely more interesting than any results to which their >> experiments >> lead. >> -- Norbert Wiener >> > > > > -- > Lisandro Dalc?n > --- > Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) > Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) > Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) > PTLC - G?emes 3450, (3000) Santa Fe, Argentina > Tel/Fax: +54-(0)342-451.1594 >
I really like "const" usage, but isn't this too much?
That is wierd; it seems strange that it would even compile.
BTW: I think your new change
7 - v->unplacedarray = v->array; /* save previous array so reset can
bring it back */ 8
- v->array = (PetscScalar *)a; 9 +
if (a) { 10 + v->unplacedarray = v->array; /* save previous array so
reset can bring it back */ 11
+ v->array = (PetscScalar *)a; 12 + }
is TOTALLY wrong. It is perfectly legitimate to place null arrays and
one wants
them to behave just like regular place arrays (saving the previous
array).
Why did you make this change and what was wrong with the way it was?
Barry
On Dec 3, 2008, at 4:45 PM, Lisandro Dalcin wrote:
> VecPlaceArray(Vec vec,const PetscScalar array[])
> VecReplaceArray(Vec vec,const PetscScalar array[])
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
column map in Mat MPIAdj
On Wed, Dec 3, 2008 at 5:28 PM, Barry Smith wrote: > > The normal pattern (for example MPIAIJ, MPIBAIJ) is for the PetscMap setup > being done in the associated MatXXXSetPreallocation() routine. > > I think the correct fix is > 1) change the ierr = MatSetSizes(*A,m,n,PETSC_DETERMINE,n);CHKERRQ(ierr); > in MatCreateMPIAdj to ierr = > MatSetSizes(*A,m,PETSC_DECIDE,PETSC_DETERMINE,n);CHKERRQ(ierr); Yep, this seems to be fix. I believe I'll even change the name of the 'n' argument to 'N' and put it VERY clear in the docs that 'N' is de global number of columns. > 2) remove the two lines > if (B->cmap->n < 0) B->cmap->n = B->cmap->N; > if (B->cmap->N < 0) B->cmap->N = B->cmap->n; > from MatCreate_MPIAdj() > > 3) either add the ierr = PetscMapSetUp(B->cmap);CHKERRQ(ierr); to > MatCreate_MPIAdj() OR put the PetscMapSetUp() for both rmap and > cmap into MatMPIAdjSetPreallocation() [this would be best cause it matches > the pattern of the others] I agree. Let's setup the maps in MatMPIAdjSetPreallocation(). > > Or am I missing something. > > Barry > > The business of setting the local column size to the global column size came > from > long ago and probably makes no sense any more. > > On Dec 3, 2008, at 9:36 AM, Lisandro Dalcin wrote: > >> Sorry, my comment about PETSC_COMM_SELF was a nonsense :-( >> >> Let me write the rule in C-Python-pseudocode >> >> rank = mat->comm->rank >> size = mat->comm->size >> >> cmap->N = # total_columns >> >> for p in range(size): >> if p == rank: >> mat->cmap->n = cmap->N >> else: >> mat->cmap->n = 0 >> >> # and finally >> PetscMapSetUp(mat->cmap) >> >> That would be fine? >> >> >> On Wed, Dec 3, 2008 at 1:19 PM, Lisandro Dalcin wrote: >>> >>> Can someone help me to review this routine? >>> >>> MatCreate_MPIAdj(Mat B) >>> >>> I understand that MATMPIADJ is rather special, but the call above is >>> never made >>> >>> PetscMapSetUp(B->cmap) >>> >>> then It seems that some stuff (ranges) in the column map will be >>> uninitialized. >>> >>> What values should have the entries in "cmap" for mpiadj matrices? >>> Should the "comm" in "cmap" be MPI_COMM_SELF, then we can call >>> PetscMapSetUp? >>> >>> >>> >>> >>> -- >>> Lisandro Dalc?n >>> --- >>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) >>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) >>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >>> Tel/Fax: +54-(0)342-451.1594 >>> >> >> >> >> -- >> Lisandro Dalc?n >> --- >> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) >> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) >> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >> Tel/Fax: +54-(0)342-451.1594 >> > > -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
Why MatSetSizes_SeqDense memzeroes values?
It's my guess, too. I'm going to push this.
On Wed, Dec 3, 2008 at 4:41 PM, Barry Smith wrote:
>
> My guess is you can/should remove the zero completely.
>
> Barry
>
> On Dec 3, 2008, at 12:09 PM, Lisandro Dalcin wrote:
>
>> Please, review the changeset below, just pushed. Why is the memzero
>> call being issued? Should it be completely removed?
>>
>> changeset: 13056:4ea189951503
>> tag: tip
>> user:Lisandro Dalcin
>> date:Wed Dec 03 16:06:12 2008 -0200
>> summary: protect memzero call in MatSetSizes_SeqDense()
>>
>>
>> I discovered the problem doing this:
>>
>> In [1]: from petsc4py import PETSc
>> In [2]: A = PETSc.Mat().create()
>> In [3]: A.setSizes([4,4])
>> In [4]: A.setType('seqdense')
>> In [5]: A.setSizes([4,4])
>>
>> ---
>> Error Traceback (most recent call
>> last)
>> Error: error code 85
>> [0] MatSetSizes() line 128 in src/mat/utils/gcreate.c
>> [0] MatSetSizes_SeqDense() line 1529 in src/mat/impls/dense/seq/dense.c
>> [0] PetscMemzero() line 189 in src/sys/utils/memc.c
>> [0] Null argument, when expecting valid pointer
>> [0] Trying to zero at a null pointer
>>
>>
>> --
>> Lisandro Dalc?n
>> ---
>> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
>> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
>> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
>> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
>> Tel/Fax: +54-(0)342-451.1594
>>
>
>
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
fixes for VecPlaceArray/VecResetArray
On Wed, Dec 3, 2008 at 5:04 PM, Lisandro Dalcin wrote: > I've pushed some fixes for VecPlaceArray/VecResetArray, trying to the > pair a bit more smarter. However, the I'm not sure what's the use case > in VecPlaceArray for accepting NULL for the array. Just in case, I did > not changed this, but if NULL is actually pased, then the call is now > non-op. Is this fine? I'm missing something?? I think you would like to reset the array back to NULL sometimes, to clear out memory of an old array it no longer owns. Is this right? Matt > > -- > Lisandro Dalc?n > --- > Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) > Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) > Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) > PTLC - G?emes 3450, (3000) Santa Fe, Argentina > Tel/Fax: +54-(0)342-451.1594 > -- What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead. -- Norbert Wiener -- next part -- An HTML attachment was scrubbed... URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20081203/b7ddf925/attachment.html>
Why MatSetSizes_SeqDense memzeroes values?
Please, review the changeset below, just pushed. Why is the memzero
call being issued? Should it be completely removed?
changeset: 13056:4ea189951503
tag: tip
user:Lisandro Dalcin
date:Wed Dec 03 16:06:12 2008 -0200
summary: protect memzero call in MatSetSizes_SeqDense()
I discovered the problem doing this:
In [1]: from petsc4py import PETSc
In [2]: A = PETSc.Mat().create()
In [3]: A.setSizes([4,4])
In [4]: A.setType('seqdense')
In [5]: A.setSizes([4,4])
---
Error Traceback (most recent call last)
Error: error code 85
[0] MatSetSizes() line 128 in src/mat/utils/gcreate.c
[0] MatSetSizes_SeqDense() line 1529 in src/mat/impls/dense/seq/dense.c
[0] PetscMemzero() line 189 in src/sys/utils/memc.c
[0] Null argument, when expecting valid pointer
[0] Trying to zero at a null pointer
--
Lisandro Dalc?n
---
Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
PTLC - G?emes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
column map in Mat MPIAdj
Sorry, my comment about PETSC_COMM_SELF was a nonsense :-( Let me write the rule in C-Python-pseudocode rank = mat->comm->rank size = mat->comm->size cmap->N = # total_columns for p in range(size): if p == rank: mat->cmap->n = cmap->N else: mat->cmap->n = 0 # and finally PetscMapSetUp(mat->cmap) That would be fine? On Wed, Dec 3, 2008 at 1:19 PM, Lisandro Dalcin wrote: > Can someone help me to review this routine? > > MatCreate_MPIAdj(Mat B) > > I understand that MATMPIADJ is rather special, but the call above is never > made > > PetscMapSetUp(B->cmap) > > then It seems that some stuff (ranges) in the column map will be > uninitialized. > > What values should have the entries in "cmap" for mpiadj matrices? > Should the "comm" in "cmap" be MPI_COMM_SELF, then we can call > PetscMapSetUp? > > > > > -- > Lisandro Dalc?n > --- > Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) > Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) > Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) > PTLC - G?emes 3450, (3000) Santa Fe, Argentina > Tel/Fax: +54-(0)342-451.1594 > -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
column map in Mat MPIAdj
The normal pattern (for example MPIAIJ, MPIBAIJ) is for the PetscMap setup being done in the associated MatXXXSetPreallocation() routine. I think the correct fix is 1) change the ierr = MatSetSizes(*A,m,n,PETSC_DETERMINE,n);CHKERRQ(ierr); in MatCreateMPIAdj to ierr = MatSetSizes(*A,m,PETSC_DECIDE,PETSC_DETERMINE,n);CHKERRQ(ierr); 2) remove the two lines if (B->cmap->n < 0) B->cmap->n = B->cmap->N; if (B->cmap->N < 0) B->cmap->N = B->cmap->n; from MatCreate_MPIAdj() 3) either add the ierr = PetscMapSetUp(B->cmap);CHKERRQ(ierr); to MatCreate_MPIAdj() OR put the PetscMapSetUp() for both rmap and cmap into MatMPIAdjSetPreallocation() [this would be best cause it matches the pattern of the others] Or am I missing something. Barry The business of setting the local column size to the global column size came from long ago and probably makes no sense any more. On Dec 3, 2008, at 9:36 AM, Lisandro Dalcin wrote: > Sorry, my comment about PETSC_COMM_SELF was a nonsense :-( > > Let me write the rule in C-Python-pseudocode > > rank = mat->comm->rank > size = mat->comm->size > > cmap->N = # total_columns > > for p in range(size): > if p == rank: > mat->cmap->n = cmap->N > else: > mat->cmap->n = 0 > > # and finally > PetscMapSetUp(mat->cmap) > > That would be fine? > > > On Wed, Dec 3, 2008 at 1:19 PM, Lisandro Dalcin > wrote: >> Can someone help me to review this routine? >> >> MatCreate_MPIAdj(Mat B) >> >> I understand that MATMPIADJ is rather special, but the call above >> is never made >> >> PetscMapSetUp(B->cmap) >> >> then It seems that some stuff (ranges) in the column map will be >> uninitialized. >> >> What values should have the entries in "cmap" for mpiadj matrices? >> Should the "comm" in "cmap" be MPI_COMM_SELF, then we can call >> PetscMapSetUp? >> >> >> >> >> -- >> Lisandro Dalc?n >> --- >> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) >> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) >> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) >> PTLC - G?emes 3450, (3000) Santa Fe, Argentina >> Tel/Fax: +54-(0)342-451.1594 >> > > > > -- > Lisandro Dalc?n > --- > Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) > Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) > Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) > PTLC - G?emes 3450, (3000) Santa Fe, Argentina > Tel/Fax: +54-(0)342-451.1594 >
column map in Mat MPIAdj
Can someone help me to review this routine? MatCreate_MPIAdj(Mat B) I understand that MATMPIADJ is rather special, but the call above is never made PetscMapSetUp(B->cmap) then It seems that some stuff (ranges) in the column map will be uninitialized. What values should have the entries in "cmap" for mpiadj matrices? Should the "comm" in "cmap" be MPI_COMM_SELF, then we can call PetscMapSetUp? -- Lisandro Dalc?n --- Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC) Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC) Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET) PTLC - G?emes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594
Why MatSetSizes_SeqDense memzeroes values?
My guess is you can/should remove the zero completely.
Barry
On Dec 3, 2008, at 12:09 PM, Lisandro Dalcin wrote:
> Please, review the changeset below, just pushed. Why is the memzero
> call being issued? Should it be completely removed?
>
> changeset: 13056:4ea189951503
> tag: tip
> user:Lisandro Dalcin
> date:Wed Dec 03 16:06:12 2008 -0200
> summary: protect memzero call in MatSetSizes_SeqDense()
>
>
> I discovered the problem doing this:
>
> In [1]: from petsc4py import PETSc
> In [2]: A = PETSc.Mat().create()
> In [3]: A.setSizes([4,4])
> In [4]: A.setType('seqdense')
> In [5]: A.setSizes([4,4])
> ---
> Error Traceback (most recent
> call last)
> Error: error code 85
> [0] MatSetSizes() line 128 in src/mat/utils/gcreate.c
> [0] MatSetSizes_SeqDense() line 1529 in src/mat/impls/dense/seq/
> dense.c
> [0] PetscMemzero() line 189 in src/sys/utils/memc.c
> [0] Null argument, when expecting valid pointer
> [0] Trying to zero at a null pointer
>
>
> --
> Lisandro Dalc?n
> ---
> Centro Internacional de M?todos Computacionales en Ingenier?a (CIMEC)
> Instituto de Desarrollo Tecnol?gico para la Industria Qu?mica (INTEC)
> Consejo Nacional de Investigaciones Cient?ficas y T?cnicas (CONICET)
> PTLC - G?emes 3450, (3000) Santa Fe, Argentina
> Tel/Fax: +54-(0)342-451.1594
>
