Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jgpallero opened this issue Jul 6, 2024 · 5 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jgpallero
Copy link

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 apjanke self-assigned this Jul 6, 2024
@apjanke apjanke added the bug Something isn't working label Jul 6, 2024
@apjanke apjanke added this to the 0.4.3 milestone Jul 6, 2024
@apjanke
Copy link
Owner

apjanke commented Jul 6, 2024

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

  • Fix this.
  • Add a unit test that will catch this.

@apjanke apjanke changed the title Warning in duration function Warning in duration constructor: "colon arguments should be scalars" Jul 6, 2024
@jgpallero
Copy link
Author

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
Copy link
Owner

apjanke commented Jul 6, 2024

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 added a commit that referenced this issue Jul 6, 2024
…tr row vectors

The `for i = 1:size (strs)` is a bug; it should be `numel (strs)`. When using `size()`, you get a vector, then I think the `1:x` colon operator just uses the first element of the RHS, so it under-counts if dim 2 or higher is nonzero, and leaves the ignored elements as NaN in the output.

See: #134
@apjanke
Copy link
Owner

apjanke commented Jul 6, 2024

Here's a proposed fix, on branch fix-duration-ctor, in commit 66ac059. (Updated version: a77a220.)

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 added a commit that referenced this issue Jul 6, 2024
…tr row vectors

The `for i = 1:size (strs)` is a bug; it should be `numel (strs)`. When using `size()`, you get a vector, then I think the `1:x` colon operator just uses the first element of the RHS, so it under-counts if dim 2 or higher is nonzero, and leaves the ignored elements as NaN in the output.

See: #134
@apjanke
Copy link
Owner

apjanke commented Jul 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Closed
Development

No branches or pull requests

2 participants