bugGNU Octave - Bugs: bug #63515, logical should convert char and...

 
 

bug #63515: logical should convert char and strings instead of throwing an error for Matlab compatibility

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Mon 12 Dec 2022 08:20:01 PM UTC
 
Category:  Octave Function Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Confirmed Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Wed 11 Jan 2023 09:45:26 PM UTC, comment #8: 

OK, I think we can change any remaining bool_array_value functions to ignore the "warn" argument.  In a separate change we could deprecate and later remove the "warn" argument.

John W. Eaton <jwe>
Project Administrator
Wed 11 Jan 2023 09:16:21 PM UTC, comment #7: 

There are only two instances in libinterp that I can find:

operators/op-bm-b.cc:82:  boolNDArray v2 = a2.bool_array_value (true);
operators/op-bm-bm.cc:123:  boolNDArray v2 = a2.bool_array_value (true);

If I look at the code in op-bm-b.cc I find

static octave_value
oct_assignop_conv_and_assign (octave_base_value& a1,
                              const octave_value_list& idx,
                              const octave_base_value& a2)
{
  octave_bool_matrix& v1 = dynamic_cast<octave_bool_matrix&> (a1);

  // FIXME: perhaps add a warning for this conversion
  //        if the values are not all 0 or 1?

  boolNDArray v2 = a2.bool_array_value (true);

  v1.assign (idx, v2);

  return octave_value ();
}

There is a FIXME note about warning if the conversion was from an array like you mentioned such as [1, 1, 2, 3].  However, these files are for operator functions that are only called when both objects are already logical values (either bool matrix or bool).  So, any testing on conversion from octave_value to a strongly typed bool class won't result in anything and is wasted effort.  Or, I should say I think this is the correct interpretation of these bits of code.

Rik <rik5>
Project Administrator
Wed 11 Jan 2023 07:23:24 PM UTC, comment #6: 

I'm thinking that optional warning is some very old code that I wrote thinking that it would be useful to warn for something like

logical ([1, 1, 2, 3])

but not

logical ([0, 1, 0, 1])

Now I'm wondering why that would ever be needed.  Is there any code in Octave today that actually sets the WARN parameter to true for this function?

John W. Eaton <jwe>
Project Administrator
Wed 11 Jan 2023 04:23:38 PM UTC, comment #5: 

@jwe: Could you take a look at my comment #3?  I think this is the right line of attack for this bug, but I want to be sure before going further.

Rik <rik5>
Project Administrator
Thu 15 Dec 2022 10:36:19 PM UTC, comment #4: 

just adding a reminder that if this gets completed we should change the 'numeric' part of

Logical values can also be constructed by casting numeric objects to logical values, or by using the true or false functions.

at https://docs.octave.org/latest/Logical-Values.html, and also in the help, once logical values are included.

Nicholas Jankowski <nrjank>
Project Member
Tue 13 Dec 2022 11:27:17 PM UTC, comment #3: 

Everything turns into a project.  I thought this would be easy, and it mostly is, but then there are all sorts of questions that arise.  To have "logical" work on single-quoted strings is the easiest.  See the attached changeset.  The tricky thing is that the function I added, bool_array_value, is probably not correct.  The function I wrote is

+  boolNDArray bool_array_value (bool = false) const
+  { return boolNDArray (m_matrix); }

But, if I look at how it is implemented in ov-re-mat.cc, it seems like the input "warn" needs to be used.

boolNDArray
octave_matrix::bool_array_value (bool warn) const
{
  if (m_matrix.any_element_is_nan ())
    octave::err_nan_to_logical_conversion ();
  if (warn && m_matrix.any_element_not_one_or_zero ())
    warn_logical_conversion ();

  return boolNDArray (m_matrix);
}

char arrays do not hold NaN elements so that test isn't needed, but the function "any_element_not_one_or_zero" is.  Unfortunately, the class charNDArray does not implement this so it would require adding it to that class which is is a change in liboctave.  All of this is doable, but I'm starting to wonder if I'm on the right track.  Adding jwe to the CC list so he can comment.

(file #54088)

Rik <rik5>
Project Administrator
Tue 13 Dec 2022 02:18:39 PM UTC, comment #2: 

thanks, that matches matlab behavior for other logical uses of chars and strings, except for the short circuit operators as was discussed over at https://octave.discourse.group/t/logical-tests-with-char-vectors/3581/22

>> if ('chars'), disp('hello'),end
hello
>> if ("str"), disp('hello'),end
Conversion to logical from string is not possible.

>> true & 'chars'

ans =

  1×5 logical array

   1   1   1   1   1

>> true & "str"
Operator '&' is not supported for operands of type 'string'.

>> true && 'chars'
Operands to the logical AND (&&) and OR (||) operators must be convertible to logical scalar
values. Use the ANY or ALL functions to reduce operands to logical scalar values.

>> true && "str"
Conversion to logical from string is not possible

so, whether or not this would get addressed before Octave has a working string object, Octave would have to decide what the desired output is. I would argue that octave should allow conversion in all cases, initially no difference between 'str' and "str", and later after implementation the only difference being that logical('str') turns into [true true true], while logical("str") would turn into [true].

Nicholas Jankowski <nrjank>
Project Member
Mon 12 Dec 2022 11:11:05 PM UTC, comment #1: 

I checked with Matlab and the conversion from a character vector to a logical vector works as expected without throwing an error.

logical ('str')
 => [1 1 1]

Trying to perform logical on a string object DOES throw an error.

logical ("str")

I've marked this as a Matlab Compatibility issue.

Rik <rik5>
Project Administrator
Mon 12 Dec 2022 08:20:01 PM UTC, original submission:  

noting elsewhere that implicit logical conversion on either singly or doubly quoted strings is performed by:  all, any, not, and, or, !, &, |, &&, and || , as well as implicitly within if conditions, it would seem consistent for

>> logical("str")

error: logical: wrong type argument 'string'

>> logical('chars')

error: logical: wrong type argument 'sq_string'

to convert into logical arrays as well instead of producing an error.

Nicholas Jankowski <nrjank>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54088:  63515.v1.cset added by rik5 (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only project members can vote.

     

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-12-13 rik5 Attached File- Added 63515.v1.cset, #54088
        Carbon-Copy- Added jwe
    2022-12-12 rik5 Severity1 - Wish 2 - Minor
        Item GroupFeature Request Matlab Compatibility
        StatusNone Confirmed
        Summarylogical should convert char and strings instead of throwing an error logical should convert char and strings instead of throwing an error for Matlab compatibility

    Back to the top

    Powered by Savane 3.10