bugGNU Octave - Bugs: bug #39532, hgload/hgsave functions missing

 
 

bug #39532: hgload/hgsave functions missing

Submitted by:  Rik <rik5>
Submitted on:  Fri 19 Jul 2013 07:34:57 PM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 25 Mar 2014 09:16:12 AM UTC, comment #13: 

Ok, thank you for your corrections. I hope I'll be able to do all of them on my own next time (before submitting the patch).

Massimiliano Fasi <mfasi>
Fri 21 Mar 2014 05:29:19 PM UTC, comment #12: 

I checked in a changeset that added hgsave/hgload here (http://hg.savannah.gnu.org/hgweb/octave/rev/13c80c3e9660).

I made some changes to match a few more of Octave's coding guidelines, but overall this was good work.

There was quite a bit of housekeeping to do with this changeset which, as a new contributor, you wouldn't be expected to know about and code for.  In this case, I added the functions to the documentation manual and wrote an intro for them, I announced the new functions in the NEWS file, I added them to the build system, and also removed them from the list of unimplemented functions, and updated other functions, such as saveas, to have seealso links to the new functions.

This will be a part of the 4.2 release.  Closing report.

Rik <rik5>
Project Administrator
Thu 06 Mar 2014 09:58:45 PM UTC, comment #11: 

Thank you again! You are spotting some problems I would have never cared about. [I have submitted a new revision of the patch that should solve the two issues you have stressed.]

(file #30806)

Massimiliano Fasi <mfasi>
Thu 06 Mar 2014 08:02:13 PM UTC, comment #10: 

After "test hgsave", I have 2 new files in my home directory :-), use "tmpnam ()" if you want to write test files and ensure they will be deleted.

Also, the following script should return an explicit error:

fname = [tmpnam(), ".ofig"];
hgsave (fname, fname);

Pantxo Diribarne <pantxo>
Project Member
Thu 06 Mar 2014 12:24:32 PM UTC, comment #9: 

I have revised the functions following your advices. I have also added a test suite to hgsave () to check the various formats. Just to keep track, there are still two open questions about the '.ofig' extension and the try/catch statement in hgload (). Of course, I am always open to improvements.

(file #30797)

Massimiliano Fasi <mfasi>
Thu 06 Mar 2014 09:11:04 AM UTC, comment #8: 

Hi Massimiliano,

The .ofig extension doesn't seem to be needed anymore if we choose to give an octave version specific name to the saved structure. I'd remove this restriction and let the user save/load his figures with whatever extension he likes, as long as he does it with octave.

I think we now need a maintainers advice here.

What else:

  • hgload:
    • the try/catch I proposed is not really clean, but I don't see another way to lookup a variable into a binary file without loading it. As "matfile" is not supported, (maybe another feature request), does someone have an idea?
    • by the way you don't need to make load return a structure. Call it without output argument and you'll have "s_oct40" loaded in your scope.
    • You should check the existence of the file in the path before trying to load it. "which" or "exist" will help here.
    • to conform to other plotting functions, you could call 'handle' -> 'htmp' and the output argument 'h'. Return 'h = htmp' only if requested (nargout >0)
    • help text: update the function signature
    • prepend your comments with "##" not "#" (reserved for inline comments).
  • hgsave:
    • check arg1 is a graphics handle and arg2 is a char.
    • same for comments and 'handle' variable name
    • add support for other formats (I'd say all binary formats supported by save function) using a second input argument.
    • update help text
Pantxo Diribarne <pantxo>
Project Member
Wed 05 Mar 2014 01:30:51 PM UTC, comment #7: 

Thank you for your feedback. I submitted a new patch that tries solve the issues you pointed out. Any further suggestion?

(file #30779)

Massimiliano Fasi <mfasi>
Wed 05 Mar 2014 11:46:24 AM UTC, comment #6: 

Sorry my mistake, I said the way you use load doesn't work ... if you don't use '-struct'.

I think it would be better to not use '-struct' and use the name of the struct returned by load as a pointer for the origin of the file.
Let me explain: matlab figs consist of a struct with the following name "hgS_070000" or "hgS_080000" (for matlab 7 or 8) saved as a .fig with matlab's binary data format.

It would be interesting for octave to use the same strategy:

  • we could then keep track of the version of octave that created the struct. As hdl2struct may evolve and break backward compatibility, this will be helpful
  • we could also detect if the struct was written by matlab in which case we shouldn't try to load it until struct2hdl is able to handle matlab structs.

It would then give you something like:

##(hgsave)
s_oct40 = hdl2struct (handle);
save ("-binary", fname, "s_oct40")

##(hgload)
try
  load (fname, "s_oct40")
catch
  error ("hgload: support only files created by hgsave from octave version 4.0 and latter")
end_try_catch

Pantxo Diribarne <pantxo>
Project Member
Wed 05 Mar 2014 10:07:15 AM UTC, comment #5: 

Yours patches look good. Here is a fast review:

  • hgsave:
    • "print_usage ()" will return to prompt so no need to make an else statement,
    • instead of "handle = gcf()" (wich will create a new figure if none exist) use "h = get (0, "currentfigure)" and test. If h is empty prompt an error message
    • I never used the '-struct' modifier in save and I may be misreading the doc, but I don't see any interest for this purpose.
  • hgload:
    • same remark about"print_usage ()"
    • the way you load the file doesn't work. It results in a structure "hdlstruct" which has one filed "hdlstruct" which itself contains the structure you want to send to struct2hdl.
    • manage file extension.

For the last point, maybe we should choose a default extension for octave figures (say .ofig) so that we could fire a warning when somebody tries to load a '.fig' that probably comes from matlab hgsave (which will fail).

Somebody has an opinion about this?

Pantxo Diribarne <pantxo>
Project Member
Wed 05 Mar 2014 08:11:11 AM UTC, comment #4: 

I have submitted a possible patch or this bug. Can someone check it and suggest me how to improve it?

Thanks.

(file #30778)

Massimiliano Fasi <mfasi>
Tue 04 Mar 2014 10:49:28 PM UTC, comment #3: 

The matlab doc says the 'all' option (and also 'property_structure' I suppose) is deprecated, so I would not bother with it.

As you mention matlab's .mat, hgsave should also support other binary formats than '-v6' and 'v7.3' (octave's '-binary' at least).  

Pantxo Diribarne <pantxo>
Project Member
Tue 04 Mar 2014 09:51:41 PM UTC, comment #2: 

May I ask, since hgsave will produce .mat files, the 'all' property is still needed? The same question goes for the
'property_structure' property in hgload function. Do these 2 still need to be implemented?

Ouzounis Georgios <gouzouni>
Tue 04 Mar 2014 05:18:24 PM UTC, comment #1: 

It should be noted that the current implementation of hdl2struct and struct2hdl don't produce matlab compatible figure structures.
If details are of any relevance here I can be more verbose.

At this stage hgsave will produce .mat files that can't be loaded in matlab using hgload and vice versa.

Pantxo Diribarne <pantxo>
Project Member
Fri 19 Jul 2013 07:34:57 PM UTC, original submission:  

Now that hdl2struct and struct2hdl are implemented, it would be quite easy to work up m-files for the functions hgload and hgsave.

Documentation for the hgload API is here (http://www.mathworks.com/help/matlab/ref/hgload.html).

Documentation for the hgsave API is here (http://www.mathworks.com/help/matlab/ref/hgsave.html).

Rik <rik5>
Project Administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #30806:  bugfix_39532-2.patch added by mfasi (5KiB - text/x-diff - After comment #10)
file #30797:  bugfix_39532-1.patch added by mfasi (5KiB - text/x-diff)
file #30779:  bugfix_39532.patch added by mfasi (4KiB - text/x-diff)
file #30778:  hgsaveandload.patch added by mfasi (3KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mfasi (Updated the item)
  • -email is unavailable- added by gouzouni (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by rik5 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-03-21 rik5 StatusPatch Submitted => Fixed
        Open/ClosedOpen => Closed
    2014-03-21 rik5 StatusNone => Patch Submitted
    2014-03-06 mfasi Attached File- => Added bugfix_39532-2.patch, #30806
    2014-03-06 mfasi Attached File- => Added bugfix_39532-1.patch, #30797
    2014-03-05 mfasi Attached File- => Added bugfix_39532.patch, #30779
    2014-03-05 mfasi Attached File- => Added hgsaveandload.patch, #30778

    Back to the top


    Powered by Savane 3.5