bugGNU Octave - Bugs: bug #47627, datenum input validation should be...

 
 

bug #47627: datenum input validation should be stricter

Submitter:  None
Submitted:  Tue 05 Apr 2016 04:36:40 PM UTC
   
 
Category:  Octave Function Severity:  2 - Minor
Priority:  3 - Low Item Group:  Documentation
Status:  Fixed Assigned to:  None
Originator Name:  Ruben Gonzalez Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 7.0.90
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 17 Jan 2022 05:10:53 PM UTC, comment #28: 

Looks good to me.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Mon 17 Jan 2022 05:09:37 PM UTC, comment #27: 

I checked in a tweak to the BIST test to get it to pass here: http://hg.savannah.gnu.org/hgweb/octave/rev/744778e9210b.

Is it okay to close this bug report again?

Rik <rik5>
Group administrator
Fri 14 Jan 2022 07:47:24 PM UTC, comment #26: 

processing /home/buildbotu/fc25-x86_64/clang-fedora/src/scripts/time/datevec.m
*** fail ("datevec ('2015-03-31 0:00','yyyy-mm-dd hh:mm')", ...
      "warning", "Format specifiers for time should be upper case");
!!!!! test failed
expected warning <Format specifiers for time should be upper case>
but got error <datevec: multiple month specifiers in yyyy-mm-dd hh:mm>

Dmitri A. Sergatskov <dasergatskov>
Fri 14 Jan 2022 07:10:28 PM UTC, comment #25: 

I like the extra warning.

The function ismember() is pretty slow as it has to do a lot of set-up work before using a compiled function like lookup().  I think a better alternative here is strchr().  I made a little benchmarking script shown below.


N = 1e4;

f = "YYYY-MM-DD HH:MM";

tic;
for i = 1:N
  #tf = any (ismember (f, "hsfYD"));
  tf = ! isempty (strchr (f, "hsfYD"));
endfor
toc


Results
ismember : 1.8 seconds
strchr   : 0.8 seconds

It's more than a factor of 2 faster so I think that is a useful change to make.

On the warning ID, the overall namespace is "Octave" and the next lower namespace is usually the name of the function.  I would propose "Octave:datevec:date-format-spec" as the warning ID.

I also thought it useful to have some BIST tests for the new warning ID.

I put all of these ideas into this changeset: http://hg.savannah.gnu.org/hgweb/octave/rev/c7a515aa767e.




octave:2> exit

Rik <rik5>
Group administrator
Fri 14 Jan 2022 05:34:36 PM UTC, comment #24: 

I pushed another patch adding the warning to the default branch here:
http://hg.savannah.gnu.org/hgweb/octave/rev/e9bfb92dab01

With it the output for some examples is:

>> datenum('2015-03-31 0:00','YYYY-MM-DD HH:MM')

warning: datevec: Format specifiers for dates should be lower case, format specifiers for time should be upper case.  Possible issue with 'm' (month) and 'M' (minutes)?
warning: called from
    datevec>__date_vfmt2sfmt__ at line 212 column 5
    datevec at line 144 column 44
    datenum at line 119 column 46

error: datevec: multiple minute specifiers in YYYY-MM-DD HH:MM
error: called from
    datevec>__date_vfmt2sfmt__ at line 268 column 5
    datevec at line 144 column 44
    datenum at line 119 column 46
>> datenum('2015-03-31','YYYY-MM-DD')

warning: datevec: Format specifiers for dates should be lower case, format specifiers for time should be upper case.  Possible issue with 'm' (month) and 'M' (minutes)?
warning: called from
    datevec>__date_vfmt2sfmt__ at line 212 column 5
    datevec at line 144 column 44
    datenum at line 119 column 46

ans = 7.3600e+05
>>


Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Fri 14 Jan 2022 04:57:12 PM UTC, comment #23: 

Thanks. Pushed here:
http://hg.savannah.gnu.org/hgweb/octave/rev/beac32c0e9c7

I'll leave this open for the opposite case warning.

Markus Mützel <mmuetzel>
Group administrator
Fri 14 Jan 2022 04:17:21 PM UTC, comment #22: 

I think Octave should always do reasonable input validation to prevent Garbage In / Garbage Out calculations.  An additional reason for tight input validation is that many users of Octave are not professional programmers (they are scientists, engineers, etc.) and so it can be very helpful to have warning messages when they are doing something that probably isn't right.

@markus: Fine with me if you push your small patch to stable branch.

Rik <rik5>
Group administrator
Wed 12 Jan 2022 04:59:49 PM UTC, comment #21: 

Thinking about it, allowing something like 'd yyyy-mmm-dd HH:MM' is kind of silly and likely a bug in user code.

Please, see the attached patch that separates the checks for day and for day of week specifiers. I'd like to push that change to the stable branch before the release of Octave 7 if no one opposes.

It might still make sense to issue a warning if specifiers appear in the "opposite case". But that change is likely for the default branch...

(file #52653)

Markus Mützel <mmuetzel>
Group administrator
Wed 12 Jan 2022 03:19:08 PM UTC, comment #20: 

Fwiw, in Matlab R2021a:

>> datenum('Wed 2015-Apr-01 00:00','ddd yyyy-mmm-dd HH:MM')

ans =

      736055

>> datenum('1 2015-Apr-01 00:00','d yyyy-mmm-dd HH:MM')

ans =

      736055


So two "d" specifiers are allowed in the same date format string.

Alternatively, to what I wrote in comment #19, we could also just remove the check for duplicate "d" specifiers...


Markus Mützel <mmuetzel>
Group administrator
Wed 12 Jan 2022 03:06:15 PM UTC, comment #19: 

Maybe, we should backout the change made here.
Instead, we could emit a warning when the format specifier contains any of "hsfYD".
Something along the lines of:
"Format specifiers for dates should be lower case, format specifiers for time should be upper case. Possible issue with 'm' (month) and 'M' (minutes)?"
That would probably have helped the original author, too.
That warning could have an id so that a user could turn it off if they prefer to not see it...

Markus Mützel <mmuetzel>
Group administrator
Tue 11 Jan 2022 06:20:45 PM UTC, comment #18: 

This change seems to have made the validation overly prohibitive.
See: https://octave.discourse.group/t/change-in-datevec-incompatible-with-ctime/2105

Re-opening and targeting 7.0.90.

Markus Mützel <mmuetzel>
Group administrator
Mon 22 Nov 2021 01:34:20 AM UTC, comment #17: 

I updated the patch to use more modern coding syntax (numel() instead of length()) and added BIST tests for all of the new input validation checks.  I checked the changeset in here: http://hg.savannah.gnu.org/hgweb/octave/rev/3e09b065779d.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 04 Aug 2021 04:29:01 PM UTC, comment #16: 

same result in v6.3.0

comment #13 patch no longer applies cleanly to tip, mainly from line shifts. updated patch applies cleanly and still passes all tests.



(file #51737)

Nicholas Jankowski <nrjank>
Group Member
Wed 04 Aug 2021 06:20:15 AM UTC, comment #15: 

Lachlan's patch has not been implemented.

Using 6.1.0

datenum('2015-04-01 0:00','YYYY-MM-DD HH:MM')
ans = 735965
datenum('2015-04-01 0:00','yyyy-mm-dd HH:MM')
ans = 736055

datenum('2015-05-01 0:00','YYYY-MM-DD HH:MM')
ans = 735965
 datenum('2015-05-01 0:00','yyyy-mm-dd HH:MM')
ans = 736085


Anonymous
Sat 28 Jan 2017 10:27:02 AM UTC, comment #14: 

The latest patch by Lachlan solved the error. Thank you for your help.
I apologize for the late reply.

-Deleted Account- <carpediem>
Sat 02 Jul 2016 01:43:23 AM UTC, comment #13: 

Aditya, I think Mike was wanting you to check for multiple occurrences of HH etc.

The attached patch is more thorough, checking that we don't have both YY and YYYY, for example.

I'm retagging from "Patch submitted" to "In progress", since this isn't a blocker for 4.2.0.

(file #37658)

Lachlan Andrew <lachlan>
Sat 30 Apr 2016 04:25:59 PM UTC, comment #12: 

IIRC, the standard date formats to be parsed (as defined inside of datevec) should show an error in case of conditions such as more than one occurrence of 'HH'. Please correct me if I am missing any possible test case. Updated diff file is attached.

(file #37030)

-Deleted Account- <carpediem>
Fri 29 Apr 2016 07:43:06 PM UTC, comment #11: 

The attached patch checks whether "MM" appears more than once in the format string. I don't know enough to know whether this is the only change needed or if there are other similar cases that should be checked for. What about more than one occurrence of "HH"?

The patch contains some style problems, please make sure to use proper indentation, whitespace, and commenting style.

Also please provide a correct commit message, see for examples https://www.gnu.org/software/octave/doc/v4.0.1/General-Guidelines.html

Mike Miller <mtmiller>
Group Member
Sun 10 Apr 2016 08:15:05 AM UTC, comment #10: 

Added a check for input if more than one MM identifiers exist. Function reports an error, as required. This patch is based on current tip, and is independent of the previous patch.

(file #36886)

-Deleted Account- <carpediem>
Sun 10 Apr 2016 06:23:09 AM UTC, comment #9: 

So basically there should be a check to see if the string has mm and MM, both present. If the condition is true, octave should return an error notifying the user about the correct format. Did I get it correctly?

-Deleted Account- <carpediem>
Sat 09 Apr 2016 03:22:46 PM UTC, comment #8: 

This isn't the right solution.  It is an error to have more than one instance of the 'MM' identifier.  See comment #2 where this was verified on Matlab.  Octave needs to produce an error when this condition is detected rather than continue on using an unknown or assumed value for the month.

Rik <rik5>
Group administrator
Sat 09 Apr 2016 02:50:46 PM UTC, comment #7: 

Please add BISTs (built-in self-test) for this cases.  See the bottom of datevec and datenum files for examples.

Carnë Draug <carandraug>
Group Member
Sat 09 Apr 2016 08:28:03 AM UTC, comment #6: 
-Deleted Account- <carpediem>
Sat 09 Apr 2016 04:40:51 AM UTC, comment #5: 

The problem was not in datenum(), but in datevec().
Upon testing, I found that that datevec failed to determine the month value at all when MM is used instead of mm, as was evident from the following testcases:

datevec('2015-12-27 0:00','yyyy-MM-dd HH:MM')
datevec('2015-11-27 0:00','yyyy-mm-dd HH:MM')
datevec('2015-11-27 0:00','yyyy-MM-dd HH:MM')

The problem was that datevec was confusing 'MM' for minutes when passed as uppercase in the string and was not determining the value of month passed in f[]. It simply defaulted to month=1 for all cases, hence, giving wrong output to datenum(), which in turn calculated wrong output itself.

After applying this patch, the function works correctly according to all the std formats to be parsed, as defined in the file.

-Deleted Account- <carpediem>
Tue 05 Apr 2016 09:44:56 PM UTC, comment #4: 

So it seems that Octave processes correct input accurately, but could do more to detect when the input is invalid.  I'm going to change the bug summary to reflect that and mark the bug as confirmed.

Rik <rik5>
Group administrator
Tue 05 Apr 2016 07:59:45 PM UTC, comment #3: 

fixing the date portion to lower case, Matlab 2015b gives the following:


>> datenum('2015-03-31 0:00','yyyy-mm-dd HH:MM')

ans =

      736054

>> datenum('2015-04-01 0:00','yyyy-mm-dd HH:MM')

ans =

      736055

>> datenum('2015-05-01 0:00','yyyy-mm-dd HH:MM')

ans =

      736085


Nicholas Jankowski <nrjank>
Group Member
Tue 05 Apr 2016 07:57:26 PM UTC, comment #2: 

just checked Matlab 2015b. errors on all three:


>> datenum('2015-03-31 0:00','YYYY-MM-DD HH:MM')
Error using datenum (line 178)
DATENUM failed.

Caused by:
    Error using matlab.internal.datetime.cnv2icudf (line 132)
    Unrecognized minute format.  Format string: YYYY-MM-DD HH:MM.

>> datenum('2015-04-01 0:00','YYYY-MM-DD HH:MM')
Error using datenum (line 178)
DATENUM failed.

Caused by:
    Error using matlab.internal.datetime.cnv2icudf (line 132)
    Unrecognized minute format.  Format string: YYYY-MM-DD HH:MM.

>> datenum('2015-05-01 0:00','YYYY-MM-DD HH:MM')
Error using datenum (line 178)
DATENUM failed.

Caused by:
    Error using matlab.internal.datetime.cnv2icudf (line 132)
    Unrecognized minute format.  Format string: YYYY-MM-DD HH:MM.


Nicholas Jankowski <nrjank>
Group Member
Tue 05 Apr 2016 05:10:15 PM UTC, comment #1: 

Do you have access to Matlab and can you test there as well?

Second, according to the documentation for datestr, your format is invalid.  The correct designation for years, months, and days is lower case. 


datenum (2015, 3, 31)
ans =  736054
datenum (2015, 4, 1)
ans =  736055
% Using lowercase yyyy, mm, dd as described in datestr
datenum('2015-03-31 00:00','yyyy-mm-dd HH:MM')
ans =  736054
datenum('2015-04-01 00:00','yyyy-mm-dd HH:MM')
ans =  736055


It does look as if the input validation could be improved in Octave.

Rik <rik5>
Group administrator
Tue 05 Apr 2016 04:36:40 PM UTC, original submission:  

the function datenum for some reason ignores the month

For example, performing a datenum for March 31st 2015 yields

datenum('2015-03-31 0:00','YYYY-MM-DD HH:MM')
ans =  735995

Now for doing it for the next day, April 1st, we get

datenum('2015-04-01 0:00','YYYY-MM-DD HH:MM')
ans =  735965

which is 30 days LOWER than the previous result. Next, use June 1st instead

datenum('2015-05-01 0:00','YYYY-MM-DD HH:MM')
ans =  735965

You get the VERY SAME RESULT, which means that the month is not factored into datenum at all.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51737:  bug_47627_datevec_validation_v2.cset added by nrjank (3KiB - application/octet-stream - refreshed patch )
file #37030:  47627.diff added by carpediem (1KiB - text/x-patch)
file #36886:  47627.diff added by carpediem (1006B - text/x-patch)
file #36871:  patch47627.diff added by carpediem (781B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by carpediem (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (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 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-17 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-01-14 mmuetzel StatusIn Progress Ready For Test
    2022-01-12 mmuetzel Attached File- Added bug47627-datevec-day-of-week.patch, #52653
    2022-01-11 mmuetzel StatusFixed In Progress
        Open/ClosedClosed Open
        Releasedev 7.0.90
    2021-11-22 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2021-08-04 nrjank Attached File- Added bug_47627_datevec_validation_v2.cset, #51737
    2019-03-03 mtmiller Carbon-CopyRemoved 80942 -
    2018-08-17 mtmiller StatusIn Progress Patch Submitted
    2016-07-02 lachlan Attached File- Added bug_47627_datevec_validation.cset, #37658
        StatusPatch Submitted In Progress
    2016-04-30 carpediem Attached File- Added 47627.diff, #37030
    2016-04-29 mtmiller Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        Item GroupIncorrect Result Documentation
        StatusConfirmed Patch Submitted
        Release4.0.1 dev
        Operating SystemMicrosoft Windows Any
    2016-04-10 carpediem Attached File- Added 47627.diff, #36886
    2016-04-09 carpediem Attached File- Added patch47627.diff, #36871
    2016-04-05 rik5 StatusNeed Info Confirmed
        Summarydatenum gives wrong result, ignores month datenum input validation should be stricter

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code