bugGNU Octave - Bugs: bug #54952, optimset (old, new) uses empty...

 
 

bug #54952: optimset (old, new) uses empty fields from 'new'

Submitter:  Olaf Till <i7tiol>
Submitted:  Sun 04 Nov 2018 12:11:14 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  siko1056
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

Sat 08 Dec 2018 11:48:17 PM UTC, comment #11: 

For check-in privileges, send an e-mail to John at jwe@octave.org.  I don't think it would be a problem.

Rik <rik5>
Group administrator
Sat 08 Dec 2018 11:59:06 AM UTC, comment #10: 

Pushed: https://hg.savannah.gnu.org/hgweb/octave/rev/55564fdb3e4d and https://hg.savannah.gnu.org/hgweb/octave/rev/d77938ba31c4

Sorry, I had the impression you had push rights.  Rik, was it possible to give Olaf those rights, or should this be discussed on the mailing list?

Done.

Kai Torben Ohlhus <siko1056>
Group Member
Sat 08 Dec 2018 10:46:12 AM UTC, comment #9: 

As for pushing, I don't seem to have the rights. So I just re-attached the changeset (now 54952.cset) with only the commit message changed according to your suggestion.

You convinced me that the changes of comment #6 can be kept, but in this case I'd suggest that the comment in
https://hg.savannah.gnu.org/hgweb/octave/file/a0b63c183d4b/scripts/optimization/__all_opts__.m#l44
should be changed from 'will call optimset' to 'may call optimset', so that the precautions against this calling of optimset (possible in external code) will not be removed from Octave later.

(file #45600)

Olaf Till <i7tiol>
Group Member
Fri 07 Dec 2018 09:38:55 PM UTC, comment #8: 

Never mind, how can earning money be a fault?  Octave development is professional hobby, right? ;-)

Olaf, your cset (file #45592) works fine with me:  bug #55089 remains fixed, no other errors test bench observed.  I would only rather refer to this bug, than to bug #55089 in the commit message. Please go ahead and push it.

Not only because I made the changes, I vote to keep the changes of comment #6.  The options of each solver are kind of constant (except for the user to change them inside the m-files).  There is simply no need to verify them at each and every call.

By reverting it, we obtain back a fragile (in the sense that errors are easily introduced, if this mechanism is not understood bug #55089) circle as explained before: "optimset" calls "__all_options__" that calls "fminunc" that calls "optimset" to verify its constants.

As protection against "bad" constants, we already have this one and works fine with an error at Octave startup:
https://hg.savannah.gnu.org/hgweb/octave/file/0a862644da84/scripts/optimization/__all_opts__.m#l62

Kai Torben Ohlhus <siko1056>
Group Member
Fri 07 Dec 2018 02:55:37 PM UTC, comment #7: 

Sorry for coming in so late -- it's my current jobs fault...

I think this should rather be fixed within optimset. One reason is Matlab-compatibility -- Matlabs optimset also preserves empty values if given as arguments. Another reason is that we expected optimset to work this way all the time, and external (optimization-)code may rely on it.

So I'd prefer the patch now attached. (55089.cset -- the commit message states what the patch does.)

Kay, it could be argued that your patch could still be left in, but I'd suggest to revert it. It's not necessary any more, and it used to be expected (see comments in _all_opts_.m) that an optimization function calls optimset to return its default options).

(file #45592)

Olaf Till <i7tiol>
Group Member
Mon 26 Nov 2018 01:00:33 PM UTC, comment #6: 
Kai Torben Ohlhus <siko1056>
Group Member
Sun 25 Nov 2018 04:00:26 PM UTC, comment #5: 

Thanks for the clarification Rik.  I provided a cset, but cannot push it right now, due to connection errors, see file #45524.


$ hg pull
pulling from ssh://siko1056@hg.sv.gnu.org/octave
Enter passphrase for key '******':
remote: abort: potentially unsafe serve --stdio invocation: ['--cwd', '/srv/hg', '-R', 'octave', 'serve', '--stdio']
abort: no suitable response from remote hg!


Kai Torben Ohlhus <siko1056>
Group Member
Sun 25 Nov 2018 03:18:27 AM UTC, comment #4: 

The persistent variable is filled by the PKG_ADD script in the optimization directory.

Rik <rik5>
Group administrator
Sun 25 Nov 2018 12:27:12 AM UTC, comment #3: 

Sorry for opening this one again, but it triggered bug #55089.

The reason is a strange recursive interplay by "scripts/optimization/__all_options__" and "scripts/optimization/optimset".  It took me some time to understand it to 95% yet.

_all_options_ contains a persistent variable "save_names" [1] that gets filled by calling a Matlab compatible feature of "optimset" [2,3], but [2] does not make it very obvious that a call like


optimset ('fsolve')


is fully legitimate.  This call invokes


fsolve ('defaults')


which in turn calls "optimset" with, guess what, EMPTY VALUES! [4]  Pure horror ^^

So my solution out of this misery was to change all instances in "scripts/optimization" like [4] to return pure structs.  I think the recursive output validation by optimset is not necessary and I support the changes made so far with the non-emptiness check, which is also R2018b compatible.

This way both calls return the correct values:


>> optimset ('fsolve')
>> fsolve ('defaults')
ans =

  scalar structure containing the fields:

    AutoScaling = off
    ComplexEqn = off
    FunValCheck = off
    FinDiffType = forward
    Jacobian = off
    MaxFunEvals = [](0x0)
    MaxIter =  400
    OutputFcn = [](0x0)
    Updating = off
    TolFun =  0.0000010000
    TolX =  0.0000010000
    TypicalX = [](0x0)



The only thing I did not find out yet was, when does this persistent variable gets filled?  Because at every start of Octave it contains the right values?!

[1] https://hg.savannah.gnu.org/hgweb/octave/file/c197655f9d28/scripts/optimization/__all_opts__.m#l29

[2]
https://hg.savannah.gnu.org/hgweb/octave/file/c197655f9d28/scripts/optimization/optimset.m#l31

[3] https://hg.savannah.gnu.org/hgweb/octave/file/c197655f9d28/scripts/optimization/optimset.m#l133

[4] https://hg.savannah.gnu.org/hgweb/octave/file/c197655f9d28/scripts/optimization/fsolve.m#l175

Kai Torben Ohlhus <siko1056>
Group Member
Sun 04 Nov 2018 05:04:53 PM UTC, comment #2: 

Thanks. I'll try to be a bit more conforming in the future...

Olaf Till <i7tiol>
Group Member
Sun 04 Nov 2018 03:13:29 PM UTC, comment #1: 

Your patch is definitey doing the correct behavior.  I checked it in here (https://hg.savannah.gnu.org/hgweb/octave/rev/c197655f9d28).

I added a BIST test so this behavior is now checked.  I also modified the check-in message to conform to Octave standards.  We write 'bug #XXXXX" rather than just '#XXXXX'.  I also added a description about what, specifically, was changed in optimset.m.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Sun 04 Nov 2018 12:11:14 PM UTC, original submission:  

With option structures 'old' and 'new',

optimset (old, new)

should return an option structure with all fields of 'old', set to the values of the respective fields in 'new' if they are present and non-empty in 'new'. This is so according to Matlab documentation:

options = optimset(oldopts,newopts) combines an existing options structure oldopts with a new options structure newopts. Any parameters in newopts with nonempty values overwrite the corresponding old parameters in oldopts.

, and it should be so according to a comment in Octaves scripts/optimization/optimset.m:

  elseif (nargs == 2 && isstruct (varargin{1}) && isstruct (varargin{2}))
    ## Set slots in old from non-empties in new.

, but in fact Octaves optimset even uses empty fields in 'new'. This makes correct merging of defaults with given options awkward (in the optim package).

I've checked that fixing this wouldn't affect any Octave function  under scripts/optimization/ .

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 #45600:  54952.cset added by i7tiol (4KiB - application/octet-stream)
file #45592:  55089.cset added by i7tiol (4KiB - application/octet-stream)
file #45359:  optimset.cset added by i7tiol (767B - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by rik5 (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 21 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-08 i7tiol Attached File- Added 54952.cset, #45600
        StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-12-07 siko1056 StatusFixed Patch Reviewed
        Open/ClosedClosed Open
    2018-12-07 i7tiol Attached File- Added 55089.cset, #45592
    2018-11-26 siko1056 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-11-25 siko1056 StatusFixed Patch Submitted
        Open/ClosedClosed Open
    2018-11-25 siko1056 Attached File- Added bug_54952_struct_options.patch, #45524
        StatusNone Fixed
        Assigned toNone siko1056
        Open/ClosedOpen Closed
    2018-11-25 siko1056 StatusFixed None
        Open/ClosedClosed Open
    2018-11-24 siko1056 Dependencies- bugs #55089 is dependent
    2018-11-04 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-11-04 i7tiol StatusNone Patch Submitted
    2018-11-04 i7tiol Attached File- Added optimset.cset, #45359

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code