Re: fix null assignment

[Jaroslav Hajek]

On Mon, Sep 22, 2008 at 8:39 PM, John W. Eaton <address@hidden> wrote:
> On 22-Sep-2008, Jaroslav Hajek wrote:
>
> | On Sat, Sep 20, 2008 at 6:09 AM, John W. Eaton <address@hidden> wrote:
> |
> | > I'd prefer to avoid adding a new assign_op.  I think we would have to
> | > be careful to also check for op_nul_asn_eq in each place where
> | > op_asn_eq is used and that is likely to cause confusion.
> |
> | Why? I think it would only require to register the handlers.
>
> I was just looking at the places where we check for op_asn_eq and
> thinking that it might be necessary to also check for op_nul_asn_eq,
> and that this could lead to a maintenance problem in the future.
>
> | Btw. I don't much like the fact that A(I,:) =[[;[];];] is also a null
> | assignment in Matlab; do we really want to copy this behaviour?
>
> No, I don't think it is necessary to have [] and [;] or [,] be exactly
> equivalent.  We could have [] (and, for strings, '' and "") be the
> only ways of specifying a special nul matrix.  But for compatibility,
> we should probably accept the [;] and [,] syntax.
>
> In any case, I don't think my patch is good enough to include at this
> point.  I'd be happy to consider your approach instead, if you are
> willing to write enough of a patch to show how it would be done.
>
> Thanks,
>
> jwe
>

Hmm, at a second glance, the new op type does not seem such a great
idea. Passing it to subsasgn breaks the consistency of subsasgn
methods with Matlab's subsasgn, and requires modifying at a lot of
places.

So attached is what I finally got starting where I left off your patch
last time. I avoided cluttering the OPERATORS/op-*.cc files with the
if-is_null_matrix-mark_as_null_matrix stuff by handling null matrices
in Array<U>->Array<T> type conversion. Unfortunately, it proved
insufficient - sometimes, conversions are bypassing this method and
done element by element (e.g. Matrix->ComplexMatrix) so I needed to
track these cases down.

Btw. I found out that Matlab is also not quite consistent and seems to
do some magic:

1. a = ones (3); a(1:2,:) = [] # works
2. a = ones(3); subsasgn (a, substruct('()',{1:2, ':'}), []) # works
3. a = ones(3); b = []; subsasgn (a, substruct('()',{1:2, ':'}), b) #
works (i.e. deletes!)
4. a = 1:3; a(1:2) = '' # works, i.e. deletes, but isn't this a nonsense?
5. a = gf(1:4, 4); a(1:2) = []  # doesn't work (i.e. you cannot delete
elements from class arrays)

what do you think about these cases? Should we follow all of them?
In particular 5. is serious, but I'm sure that if we come up with a
solution, Matlab will likely solve it differently.

Apart from this patch, I have one more idea that may be better:

subclass
octave_null_matrix : octave_matrix
and
octave_null_string : octave_str_char_matrix

register them as dynamic types, and register appropriate assignment
and conversion methods, and then completely separate Array<T>::assign
and Array<T>::maybe_delete_elements (i.e. do not call the latter from
the former). This approach may require more changes than the attached
patch (though maybe not), but has the advantage that it would keep all
the null matrices & strings mess out of liboctave, which is good from
maintenance point of view if we are to change things in the future
(and I'm sure we are, because of the bugs above that Matlab should
fix).

What do you think about this approach? I could try it instead of the
attached version.


-- 
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

null-assign1.diff
Description: Text Data