bugGNU Octave - Bugs: bug #52785, some inconsistence in function...

 
 

bug #52785: some inconsistence in function 'lookup' and 'logspace' help text

Submitter:  None
Submitted:  Tue 02 Jan 2018 07:49:52 AM UTC
   
 
Category:  Documentation Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Documentation
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.1
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 08 Jan 2018 01:44:49 AM UTC, comment #8: 

@Dan: I pushed your lookup changes here (http://hg.savannah.gnu.org/hgweb/octave/rev/a00033be2134).

Rik <rik5>
Group administrator
Sat 06 Jan 2018 11:20:22 PM UTC, comment #7: 

Attached is a changeset making the change to how N is used.  It also removes what looks like a duplicate test on some local variable.  I then saw a FIXME in there about cruft related to case-insensitive greater-than and less-than test, so I looked at the background of that and decided that can go.  The code are fairly short routines that can be re-coded without too much thought, plus JWE introduced the use of C++ classes five to eight years ago such that the lexicographical comparisons are done deep within Array or its relatives.  So either that lexicographic test would have to be done there, or a more local solution would be to first convert 'table' and 'y' to upper or lower-case before applying the lookup methods.

However, as regards to the actual use of case-insensitive code in the app, I found the following references to the use of an 'i' option around 2010, and I conclude this 'i' option isn't coming back:

http://lists.gnu.org/archive/cgi-bin/namazu.cgi?query=lookup+case+insensitive&submit=Search%21&idxname=octave-maintainers&max=20&result=normal&sort=score

http://lists.gnu.org/archive/html/octave-maintainers/2010-10/msg00134.html

http://hg.savannah.gnu.org/hgweb/octave/rev/30f54b3b9953

http://hg.savannah.gnu.org/hgweb/octave/rev/566249cf97fb

http://hg.savannah.gnu.org/hgweb/octave/rev/48cb431db87b

The main issue was that applying toupper or tolower inherently introduces a scrambling of the 'table' strings which puts it in the "undefined behavior" category.  So really, if a user is interested in case-insensitive analysis s/he would be best using toupper and tolower and sorting prior to the lookup() routine.

(file #42862)

Dan Sebald <sebald>
Sat 06 Jan 2018 04:21:18 PM UTC, comment #6: 

The answer is:
                                      < M A T L A B (R) >
                            Copyright 1984-2017 The MathWorks, Inc.
                             R2017b (9.3.0.713579) 64-bit (glnxa64)
                                       September 14, 2017

 
To get started, type one of these: helpwin, helpdesk, or demo.
For product information, visit www.mathworks.com.
 

>> linspace (-0,0,5)


ans =

     0     0     0     0     0


Michael Godfrey <godfrey>
Group Member
Sat 06 Jan 2018 03:30:09 PM UTC, comment #5: 

I asked for someone to test on Matlab the following code:


linspace (-0,0,5)


I don't think we want to mess with 'l' and 'r'.  interp2 and ppval both use 'lr' in combination, and ismember is using 'm' and 'b'.  In order to do away with those options in lookup it would require carefully analyzing a lot of code I would prefer to leave alone.

If you can make a documentation patch for the definition of 'n' issue that would be good.

Rik <rik5>
Group administrator
Sat 06 Jan 2018 09:42:35 AM UTC, comment #4: 

Just a couple things related to these changesets.

First, in the "Does-anyone-really-care?" category, what should be the result for the following?  It doesn't look evenly spaced (chuckle):


octave:20> linspace(-0, 0, 5)
ans =

  -0   0   0   0   0


More seriously, the documentation for lookup() indicates


"If 'y(i) >=
     table(end)' or 'isnan (y(i))' then 'idx(i)' is 'n'."


It's a couple paragraphs later where 'n' is defined, as


"The complexity of the lookup is O(M*log(N)) where N is the size of
     TABLE and"


Perhaps the definition should be brought to just after the first use of 'n'.  Or, instead of 'n' in the first instance, it could be "then 'idx(i)' is 'size(table)'" since the expression table(end) was used.

To be technically clear, the following option is "extended to minus infinity for table increasing and infinity for table decreasing" rather than just "extended to infinity".


     'l'
          Left.  For numeric lookups the leftmost subinterval shall be
          extended to infinity (i.e., all indices at least 1).


i.e.,


octave:64> lookup([1 2 3], -inf)
ans = 0
octave:65> lookup([1 2 3], -inf, 'l')
ans =  1
octave:70> lookup([3 2 1], inf)
ans = 0
octave:71> lookup([3 2 1], inf, 'l')
ans =  1


but I guess the current documentation captures the gist of it.

I'm sort of wondering about the usefulness of 'l' and 'r' options, as these two kind of obviate the first and/or last table boundary.  Instead of using 'l', one could just add one to the non-'l' result:


octave:72> lookup([1 2 3], 0.5, 'l')
ans =  1
octave:73> lookup([1 2 3], 1.5, 'l')
ans =  1
octave:74> 1 + lookup([2 3], 0.5)
ans =  1
octave:75> 1 + lookup([2 3], 1.5)
ans =  1


That is, the thought that comes to mind is that someone is putting a bogus left (right) boundary in his or her table just so s/he can use the 'l' option.  And if that person is going to add another boundary at the front of the array, s/he must think about the increasing/decreasing aspect of it.  Also, one could do the following:


octave:126> lookup([-inf 2 inf], 0)
ans =  1
octave:127> lookup([-inf 2 inf], 3)
ans =  2


and achieve a similar result to 'lr'.  That is, rather than put in some arbitrary left and right extra boundary and use 'lr', just use -inf and inf.

Dan Sebald <sebald>
Fri 05 Jan 2018 05:32:43 AM UTC, comment #3: 

So many pesky little things to fix.  I changed the remaining instance of "val" to "y" here http://hg.savannah.gnu.org/hgweb/octave/rev/d68d1096d294.

Rik <rik5>
Group administrator
Fri 05 Jan 2018 12:53:09 AM UTC, comment #2: 

Hi,
in the newest version there is one 'val' left which should be 'y' as well.


--- a/libinterp/corefcn/lookup.cc
+++ b/libinterp/corefcn/lookup.cc
@@ -222,7 +222,7 @@
 otherwise, @code{idx(i)} is zero.

 @item b
-Boolean.  @code{idx(i)} is a logical 1 or 0, indicating whether @code{val(i)}
+Boolean.  @code{idx(i)} is a logical 1 or 0, indicating whether @code{y(i)}
 is contained in table or not.

 @item l


Anonymous
Thu 04 Jan 2018 05:51:12 AM UTC, comment #1: 

I fixed the issues identified in this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/5188d936c79a).

I also re-wrote the linspace documentation to be clearer.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Tue 02 Jan 2018 07:49:52 AM UTC, original submission:  

help lookup:

...
     If OPTS is specified, it must be a string with letters indicating
     additional options.

     'm'
          'table(idx(i)) == val(i)' if 'val(i)' occurs in table;
          otherwise, 'idx(i)' is zero.

     'b'
          'idx(i)' is a logical 1 or 0, indicating whether 'val(i)' is
          contained in table or not.
...


There is no definition of variable 'val'.
'val' might be 'y' according to the context.


help logspace:

...
     Also for compatibility with MATLAB, return the second argument B if
     fewer than two values are requested.
...


The behavior of logspace is that
if N==1, it returns 10^B or pi, but not 'B'.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  •  

    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 group members can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-06 sebald Attached File- Added octave-lookup_doc_change_code_cleanup-djs2017jan06.patch, #42862
    2018-01-04 rik5 StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code