bugGNU Octave - Bugs: bug #31392, fix for case insensitivity of...

 
 

bug #31392: fix for case insensitivity of optimset options

Submitter:  Olaf Till <i7tiol>
Submitted:  Thu 21 Oct 2010 09:46:36 AM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  highegg
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 06 Jan 2011 08:23:07 AM UTC, comment #11: 

I checked in the following changeset:

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

John W. Eaton <jwe>
Group administrator
Wed 22 Dec 2010 01:36:29 PM UTC, comment #10: 

Yes, it does. Thanks.

BTW, in optimset, the "i" option to lookup should be removed, too. Jaroslav removed this option from lookup after a discussion since it can't work as intended. It does not matter at the moment, but it will give problems if lookup one day should test if an option is valid.

Olaf Till <i7tiol>
Group Member
Wed 22 Dec 2010 09:14:04 AM UTC, comment #9: 

I checked in the following change:

http://hg.savannah.gnu.org/hgweb/octave/rev/ed827ffa5a43

With this, your example works for me.  Does this fix the problem for you?

John W. Eaton <jwe>
Group administrator
Tue 21 Dec 2010 07:14:38 AM UTC, comment #8: 

Maybe this bug gets forgotten because it is still closed and marked fixed.

It would be unlucky if it survived into the coming stable release, since the last correction is obvious and simple (see last comment).

With this bug I still can't base some functions in package optim on optimset.

Olaf Till <i7tiol>
Group Member
Mon 13 Dec 2010 11:00:45 AM UTC, comment #7: 

There is still a small mistake that prevents it from working: The lookup is done in uppercase, but the stored sort-order is in lowercase:

in __all_opts__.m:

    [lnames, idx] = unique (tolower (names));
...
      names = names(idx);


It must be both uppercase or both lowercase, otherwise, e.g.:


octave:1> munlock ("__all_opts__")
octave:2> clear __all_opts__ # for reproducibility
octave:3> function ret = test_options (arg)
> ret = optimset ("a_b", 1, "ab", 2, "c", 3);
> endfunction
octave:4> __all_opts__ ("test_options")
ans =

{
  [1,1] = a_b
  [1,2] = ab
  [1,3] = c
}

octave:5> optimset ("ab", 1);
warning: unrecognized option: ab
octave:6>


And a typo: lookup is still called with "i" in optimset.m

Olaf Till <i7tiol>
Group Member
Sun 12 Dec 2010 09:00:06 PM UTC, comment #6: 

OK, I understand the problem now. I've removed the "i" option from lookup and use explicit toupper conversions everywhere:
http://hg.savannah.gnu.org/hgweb/octave/rev/30f54b3b9953
Thanks for bringing this up.

Jaroslav Hajek <highegg>
Mon 15 Nov 2010 08:14:11 AM UTC, comment #5: 

It is fixed for the special case of my example, but here is a different example which does not work:

octave:1> munlock ("__all_opts__")
octave:2> clear _all_opts_ # for reproducibility
octave:3> function  ret = test_options (arg)

> ret = optimset ("a_b", 1, "ab", 2, "c", 3);
> endfunction

octave:4> _all_opts_ ("test_options")
ans =

{
  [1,1] = a_b
  [1,2] = ab
  [1,3] = c
}

octave:5> optimget (struct (), "a_b")
warning: unrecognized option: a_b
ans = [](0x0)
octave:6>

The reason is that lookup(,, "i") internally converts to uppercase (you have mistaken this in your last comment), while _all_opts_.m maintains the names in lowercase sort order. This doesn't matter as long the names consist only of letters, but underscore, e.g., is in between uppercase and lowercase letters in ASCII sort order.

If you change "tolower" to "toupper" in _all_opts_.m, it will work. But only as long as lookup(,, "i") indeed internally converts to uppercase (and nobody decides to change this to lowercase for some reason).

I understand that it makes sense from the point of efficiency (intended in the future as you say) to use lookup(,, "i") with an accordingly pre-sorted table. But if you keep the "i" option of lookup for this reason, you should IMHO explicitly advertize it to the user as "uppercased lookup", not "case-insensitive lookup", since the user has to know in which order the table for lookup has to be provided.

Olaf Till <i7tiol>
Group Member
Sun 14 Nov 2010 09:05:49 PM UTC, comment #4: 

Fixed upstream. I also don't quite understand your comment about sorting not being defined for lookup. Case-insensitive comparison defines what "sorted" means pretty well, only with possibly equal but not identical keys.
The idea behind lookup (t, v, "i") is that although it is equivalent to lookup (lower(t), lower(v)), the latter always converts the whole table and thus a single binary search is no longer logarithmic. In fact, this is currently also true for lookup(..., "i"), but it may be optimized in the future, and in fact it was optimized in the past, but then I did some massive code refactoring and this wasn't retained for technical reasons. I certainly intend to implement the optimization again if I have time in the future.

Jaroslav Hajek <highegg>
Sun 14 Nov 2010 08:30:25 PM UTC, comment #3: 

I think this is just a normal bug and there is a much simpler fix. The case-insensitive lookup and sort are intentional and used precisely for avoiding duplicate options differing only in case.

Jaroslav Hajek <highegg>
Wed 27 Oct 2010 10:02:44 AM UTC, comment #2: 

Not keeping it could slightly change the behavior intended to be seen by users.

Before the fix, it was intended to be an error if two optimization-functions registered the same option in different case (according to the error message in _all_opts_.m).

It can't be an error any more after the fix if the originial list with the "correct" case is not kept.

Since the optimization functions can be queried by the user for their available options, in the same way as they are queried for registering them, the user might then see the same option in different case if the programmer of one optimization function was not careful to supply the "correct" case. This might confuse the user if he is not aware of case-insensitivity.

This could be avoided if one gave up the notion of a "correct" case entirely and provided that even in queries, by a user or for registration, only lowercase is returned. But then some of the Matlab-compatible options are possibly difficult to read in the result of a user query. Also, former Matlab users might be used to see the option in the "correct" case. And the "correct" case is used in the documentation of existing optimization functions.

Olaf Till <i7tiol>
Group Member
Wed 27 Oct 2010 07:57:44 AM UTC, comment #1: 

If the options are always supposed to be handled in a case-insensitive manner, is there any reason not to just downcase the list and keep only that?  Why keep the original list?

John W. Eaton <jwe>
Group administrator
Thu 21 Oct 2010 09:46:36 AM UTC, original submission:  

Example of wrong behavior (option "Ab" should not be unrecognized after registering):

octave:1> function ret = test_options (arg)

> ret = optimset ("Ab", 1, "aa", 2, "c", 3);
> endfunction

octave:2> _all_opts_ ("test_options");
octave:3> optimget (struct (), "aa")
ans = [](0x0)
octave:4> optimget (struct (), "Ab")
warning: unrecognized option: Ab
ans = [](0x0)
octave:5>

Correct behavior with the attached changeset:

octave:1> function ret = test_options (arg)

> ret = optimset ("Ab", 1, "aa", 2, "c", 3);
> endfunction

octave:2> _all_opts_ ("test_options");
octave:3> optimget (struct (), "aa")
ans = [](0x0)
octave:4> optimget (struct (), "Ab")
ans = [](0x0)
octave:5>

The reason is that _all_opts_.m, optimset.m, and optimget.m, when dealing with the stored table of all options, use case insensitive lookup with lookup(..., ..., "i"), which is not meaningful IMHO since lookup() requires a sorted table, while case-insensitivity (option "i") makes the sorting criteria undefined, so in general the stored options will not be correctly sorted for this lookup (I'll open a thread for lookup(..., ..., "i") on the maintainers list).

The changeset replaces the use of the "i" lookup option with maintaining an additional stored set of all options sorted in lowercase.

(Note that, while _all_opts_.m, optimset.m, and optimget.m were tested from current tip, the lookup() functionality used in testing was from 3.3.51; but the decisive parts of lookup() have not changed up to current tip and the principal objections against its "i" option remain valid for current tip.)

Olaf Till <i7tiol>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #21738:  optim_options_case_insensitivity.changeset added by i7tiol (4KiB - 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 highegg (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by i7tiol (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 group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-11-14 highegg StatusNone Fixed
        Open/ClosedOpen Closed
    2010-11-14 highegg Assigned toNone highegg
    2010-10-21 i7tiol Attached File- Added optim_options_case_insensitivity.changeset, #21738

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code