Re: Patch for datetick.m to improve ML compatibiity

[Philip Nienhuis]

John W. Eaton wrote:
> On 20-Jan-2010, Philip Nienhuis wrote:
>
> | While Matlab's datetick.m can handle plots without any actual argument 
> | supplied (and then defaults to "x-axis" and an optimal tick format for 
> | the tick range found), octave's datetick.m currently insists on having 
> | at least argument "FORM" specified.
> | 
> | A bit surprising as:
> | 
> | (1) After the test for presence of argument FORM, in subfunction 
> | __datetick__, that very argument is first set to [] (empty) around line 80;
> | 
> | (2) A bit further down the logic to determine optimal tick formats has 
> | been implemented already - and seems to be used too, still further down.
> | 
> | A simple patch to comment out the lines hampering ML compatibility is 
> | attached.
> | I tested it and it seems to work OK.
> | Hopefully the diff is in an acceptable format.
>
> It's not.  A simple diff can't be applied automatically by patch.  You
> need to send a context diff.  If you are using the Mercurial sources,
> then you should send a Mercurial changeset instead of a plain context
> diff.

I'll make a context diff then - I do not use Mercurial.

> | In the patch I've also added a line (currently commented out) to set the 
> | month format to "19" (i.e. dd/mm) rather than "6" (mm/dd) for in case 
> | datetick.m determines that the ticks should be represented by month 
> | values; European users might prefer this.
> | Obviously it might be better to interrogate the system's language 
> | settings and then decide on this particular format but I wouldn't know 
> | how to do that - let alone how to do that in a platform-independent way :-(
>
> | 38,40c38,40
> | <   if (nargin < 1)
> | <     print_usage ();
> | <   else
> | ---
> | > #  if (nargin < 1)
> | > #    print_usage ();
> | > #  else
> | 48c48
> | <   endif
> | ---
> | > #  endif
>
> If you are changing this function so that it can accept any number of
> arguments, then you should remove the code and reindent, instead of
> commenting out some section.

Yes I realized later that my patch is perhaps too straightforward, sorry.

> | 107,113c107,113
> | <   if (isnumeric (form))
> | <     if (! isscalar (form) || floor (form) != form || form < 0)
> | <       error ("datetick: expecting form argument to be a positive integer");
> | <     endif
> | <   elseif (! ischar (form) && !isempty (form))
> | <     error ("datetick: expecting valid date format string");
> | <   endif
> | ---
> | > #  if (isnumeric (form))
> | > #    if (! isscalar (form) || floor (form) != form || form < 0)
> | > #      error ("datetick: expecting form argument to be a positive integer");
> | > #    endif
> | > #  elseif (! ischar (form) && !isempty (form))
> | > #    error ("datetick: expecting valid date format string");
> | > #  endif
>
> Shouldn't we still have checks if FORM is provided?  I guess this
> should be instead inside an 
>
>  if (! isempty (form)) ... endif
>
> block.

Sure, simply commenting out was to rough indeed.

> | 196c196,197
> | <       form = 6;
> | ---
> | >       form = 6;                  # for US users
> | > 	# form = 19;		# for European users  
>
> Instead of adding a line that is commented out, I'd prefer to see
>
>   ## FIXME -- FORM should be 19 for European users who use dd/mm
>   ## instead of mm/dd.  How can that be determined automatically?

OK.

> I checked in the following change to fix these problems:
>
>   http://hg.savannah.gnu.org/hgweb/octave/rev/c2f1cdb59821
>
> Now datetick succeeds with no arguments, but I don't think it is
> generating the correct tick labels.  I'm seeing
>
>  00:00  05:00  10:00  15:00  20:00  01:00
>
> What should the labels be when the x-axis range is [0, 1], and what
> change in datetick is needed?

Indeed it's not what one would expect for [0 1], although not completely 
wrong either. Obviously the interval rounding needs fine-tuning and I 
suppose that just simple adaptations are needed. I didn't touch those 
parts of the code however (yet).

Actually, IMO the entire logic of datetick() may be improved. Although 
the various blocks seem soundly written, I have a bit of trouble 
correlating the bits and pieces across the entire function - there's 
hardly any comment in it either.
Based on the mailing list archives on datetick() I suspect the way it 
has been constructed might have been intended to serve dateaxis() but is 
still left in an intermediate state.

Next weekend I'll delve a bit deeper in it.

Thank you,

Philip