apjanke / octave-tablicious

Table (relational, tabular data) implementation for GNU Octave
https://apjanke.github.io/octave-tablicious/
GNU General Public License v3.0
28 stars 11 forks source link

Warning in duration constructor: "colon arguments should be scalars" #134

Closed jgpallero closed 3 weeks ago

jgpallero commented 3 weeks ago

I'm working with tablicious' duration function with input as text mode and I can see that in this format a warning is emitted:

duration('23:55:19')
warning: colon arguments should be scalars
warning: called from
   parseTimeStringsToDatenum at line 563 column 7
   duration at line 133 column 25

ans =
23:55:19

Otherwise it works OK.

apjanke commented 3 weeks ago

Thanks for the bug report, @jgpallero.

This looks like an easy fix. I think I can get this out in a patch release in the next few days.

Diagnosis

I can reproduce this, with Tablicious 0.4.2 under Octave 8.4.0 on macOS 14 (Apple Silicon).

>> ver
----------------------------------------------------------------------
GNU Octave Version: 8.4.0 (hg id: 78c13a2594f3 + patches)
Octave.app Version: 8.4.0
GNU Octave License: GNU General Public License
Operating System: Darwin 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64
----------------------------------------------------------------------
Package Name  | Version | Installation directory
--------------+---------+-----------------------
     doctest  |   0.8.0 | /Users/janke/Library/Application Support/Octave.app/8.4.0/pkg/doctest-0.8.0
  tablicious  |   0.4.2 | /Users/janke/Library/Application Support/Octave.app/8.4.0/pkg/tablicious-0.4.2
     testify  |   0.3.3 | /Users/janke/Library/Application Support/Octave.app/8.4.0/pkg/testify-0.3.3
>> pkg load tablicious
>> which duration
'duration' is a class constructor from the file /Users/janke/Library/Application Support/Octave.app/8.4.0/pkg/tablicious-0.4.2/@duration/duration.m
>> duration('23:55:19')
warning: colon arguments should be scalars
warning: called from
    parseTimeStringsToDatenum at line 563 column 7
    duration at line 133 column 25

ans =
23:55:19
>>

That “parseTimeStringsToDatenum” is a static method inside the duration.m classdef file inside Tablicious. Looking at the source code there, around line 563:

      for i = 1:size (strs)
        strIn = strs{i};

Oopsie. That should be “1:numel(strs)”, not “1:size(strs)”. Rookie mistake. And this bug might well produce incorrect results instead of just raising a warning.

That’s an easy one-liner fix. And it’s about time to roll out a new Tablicious release anyway.

TODO for me

jgpallero commented 3 weeks ago

Hi,

I add some strange behavior of duration. In this case, when I try to use as input argument a cell array of strings. These are the results in Matlab:

>> duration({'41:22:15:14','12:05:16:30'})
ans = 
  1×2 duration array
   1006:15:14    293:16:30
>> duration({'41:22:15:14';'12:05:16:30'})
ans = 
  2×1 duration array
   1006:15:14
    293:16:30

but in Octave:

>> duration({'41:22:15:14','12:05:16:30'})
warning: colon arguments should be scalars
warning: called from
    parseTimeStringsToDatenum at line 563 column 7
    duration at line 133 column 25

ans =
41 days 22:15:14   NaT
>> duration({'41:22:15:14';'12:05:16:30'})
warning: colon arguments should be scalars
warning: called from
    parseTimeStringsToDatenum at line 563 column 7
    duration at line 133 column 25

ans =
41 days 22:15:14
12 days 05:16:30

Apart of the warning, when I use the cell with comma as elements separator the function returns a NaT signal for the second value

apjanke commented 3 weeks ago

when I use the cell with comma as elements separator the function returns a NaT signal for the second value

I think this is another effect of the size vs. numel bug. When you use a nonscalar value on the RHS of the ":" in a for loop, maybe it just grabs the first element or something.

E.g.:

>> x = [1.1 2.2];
>> for i = 1:size(x); disp(x(i)); end
warning: colon arguments should be scalars
1.100000000000000
>> x = [1.1; 2.2];
>> for i = 1:size(x); disp(x(i)); end
warning: colon arguments should be scalars
1.100000000000000
2.200000000000000
>>

When I replace that size() with numel(), seems to work right-er:

>> duration({'41:22:15:14', '12:05:16:30'})
ans =
41 days 22:15:14   12 days 05:16:30
>> duration({'41:22:15:14'; '12:05:16:30'})
ans =
41 days 22:15:14
12 days 05:16:30
>>
apjanke commented 3 weeks ago

Here's a proposed fix, on branch fix-duration-ctor, in commit https://github.com/apjanke/octave-tablicious/commit/66ac059478688243408267517edf36fa9968607c. (Updated version: https://github.com/apjanke/octave-tablicious/commit/a77a220ff95c96985862e629f6b7143c9424e843.)

After:

>> duration({'41:22:15:14', '12:05:16:30'})
ans =
41 days 22:15:14   12 days 05:16:30
>> duration({'41:22:15:14'; '12:05:16:30'})
ans =
41 days 22:15:14
12 days 05:16:30
>>
apjanke commented 3 weeks ago

I'm pretty confident this is a good fix, and feeling impatient today, plus the other changes on main have been stewing for a while and ready to go out. So I merged this to main, and rolled a Tablicious 0.4.3 release.

The release is published on GitHub now; working on getting it in to the Octave Packages index; here's gnu-octave/packages PR #479 for it.

Closing this as Fixed. Please re-open this issue if it's still broken on the main branch or in the 0.4.3 release.