Re: fix null assignment

[Jaroslav Hajek]

On Sat, Sep 20, 2008 at 6:09 AM, John W. Eaton <address@hidden> wrote:
> On 19-Sep-2008, Jaroslav Hajek wrote:
>
> | On Fri, Sep 19, 2008 at 6:50 PM, Jaroslav Hajek <address@hidden> wrote:
> | > On Fri, Sep 19, 2008 at 5:50 PM, John W. Eaton <address@hidden> wrote:
> | >> On 19-Sep-2008, Jaroslav Hajek wrote:
> | >>
> | >> | It seems that there is a need to propagate the information (whether
> | >> | this is a deletion statement) fairly deeply - thinking of things like
> | >> | a(3).x.y(1:2,:) = []
> | >> | that should work... this stuff is resolved by subsasgn, and creating
> | >> | methods like subsnulasgn would probably mean a lot of duplication. It
> | >> | seems better to make a special kind of object for the "genuine" empty
> | >> | matrix that was not touched by expressions. I'm just not sure where to
> | >> | put it.
> | >>
> | >> I think there are a few things to consider.  First, for an expression
> | >> like "X(I) = []", if X is a class object, the subsasgn method for the
> | >> class is still called.  So in that case, if we have the parser
> | >> recognize "= []" as a special case and convert it to a call to a
> | >> function that deletes elements, then that function will need to
> | >> dispatch to the subsasgn method for the given class.
> | >>
> | >> Another possibility is to tag matrices that are created with [], then
> | >> use that information in the assign2 and assignN functions to decide
> | >> when to call maybe_delete_elements.  So instead of
> | >>
> | >>  if (rhs_nr == 0 && rhs_nc == 0)
> | >>    {
> | >>      lhs.maybe_delete_elements (idx_i, idx_j);
> | >>    }
> | >>
> | >> in the assign2 function in Array.cc, we would have something like
> | >>
> | >>  if (rhs.is_null_array ())
> | >>    {
> | >>      lhs.maybe_delete_elements (idx_i, idx_j);
> | >>    }
> | >>
> | >> But is this the right place for the check?  Maybe not, since it only
> | >> happens when the RHS is [] (i.e., double) and the LHS can be anything
> | >> (even a struct array).  So it should probably be handled a little
> | >> higher up, and the detection and call to maybe_delete_elements should be
> | >> removed from the assign2 and assignN functions.  In that case, I think
> | >> the proper places for this are the various octave_value subsasgn
> | >> methods.  Then the special tag for the [] value could go in the
> | >> octave_value object itself.  Perhaps there could even be a special
> | >> octave_value type for this object, similar to the octave_magic_colon
> | >> type.  In any case, the special nature of this value/type should not
> | >> be propagated by assignment or copying since
> | >>
> | >>  x(idx) = []
> | >>
> | >> deletes, but
> | >>
> | >>  y = []; x(idx) = y
> | >>
> | >> does not.
> | >>
> | >> Unless someone else is interested, I'll take a look at making these
> | >> changes.
> | >>
> | >
> | > I was about to look at this; however, if you have a better idea, I'd
> | > say you go ahead.
> | >
> | > My aim was to avoid checking for some special value in the
> | > octave_value assignment operator & copy constructor (as you have said,
> | > it should not propagate through by copying). octave_values are copied
> | > around a lot and checking stuff there just doesn't seem nice to me.
> | > My idea was different:
> | > Add a new octave_value::assign_op (say, octave_value::op_nul_asn_eq)
> | > and simply register proper handlers for it. Then, subclass
> | > tree_null_matrix from tree_constant, and have the
> | > tree_simple_assignment with etype = op_asn_eq mutate into
> | > op_nul_asn_eq when it detects that the rhs is a tree_null_matrix
> | > instance. The calls to maybe_delete_elements will be removed from
> | > assignX and called directly.
> | >
> | > What do you think about this?
> |
> | OK, I see this is not that easy. Currently, the various operators in
> | octave_value::assign_op have no handlers actually registered; instead,
> | they are always decomposed in octave_value::assign and they cannot be
> | propagated down through subsagn & numeric_assign. This is even marked
> | as a FIXME in ov.cc; perhaps it is time to do it now?
> |
> | I'd like to know your opinion on this before I start to do anything
> | with this matter.
>
> 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. But that
would need the operator to be propagated down the subsasgn call chain.
It seems that register_assign_op and lookup_assign_op were created
precisely for this kind of work - dispatching different kinds of
assignments, and the null assignment does seem to me as a special kind
of assignment.
Currently, the code in ov.cc _assumes_ that the only assignments are =
and OP=, wit OP a binary operation. The computed assignments are being
decomposed already in ov.cc.  In fact, lookup_assign_op is never
called on anything else than op_asn_eq (apart from register_assign_op,
but that doesn't count). It looks like most of the machinery is there,
but is not used.
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? I
mean, perhaps we should restrict the null assignment to the case "=[]"
(modulo whitespace).

> Please take a look at the two sets of diffs below.
>
> The first set of changes seem to have the desired effect, but I'm not
> entirely happy with them.
>
> The second set was my attempt to move the nul_matrix flag up to just
> the NDArray and charMatrix classes (that part is not complete) by
> modifying the assignment operations so that [] (or "") on the RHS of
> an assignment is trapped in the assignment operator itself, and then
> calling maybe_delete_elements directly from there rather than from the
> assignX functions.  This set of changes does not work properly.  For
> example, given x = {1, 2, 3}, x(1) = []  produces {[], 2, 3} instead
> of {2, 3}.
>
> I'm tired of looking at this problem now.  I'd probably go for some
> variation on the first change unless you have a better idea.
>
> jwe
>
>



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