Bug 145578 - The WEEKS() function gives an incorrect answer around January 4th
Summary: The WEEKS() function gives an incorrect answer around January 4th
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Winfried Donkers
URL:
Whiteboard: target:7.4.0 target:7.3.0.0.beta2
Keywords: difficultyBeginner, easyHack, skillCpp, topicDebug
Depends on:
Blocks: Calc-Function
  Show dependency treegraph
 
Reported: 2021-11-07 17:14 UTC by Yuri Pieters
Modified: 2022-05-02 16:54 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Pieters 2021-11-07 17:14:21 UTC
Description:
The WEEKS function appears to be giving an incorrect answer when calculating a calendar week difference before January 4th of a year.

I've tried to work through the calculation as given in the source code separately, and when I did this I got the correct answer, so I'm not sure where the issue could be coming from.

Steps to Reproduce:
1. Put "=WEEKS("2022-01-09","2022-01-10",1)" in a cell
2. Put "=WEEKS("2022-01-02","2022-01-03",1)" in a second cell

I found another set of dates that illustrate the problem too:
"=WEEKS("2017-01-08","2017-01-09",1)"
"=WEEKS("2017-01-01","2017-01-02",1)"

Actual Results:
The first cell shows 1, the second one shows 0.

Expected Results:
Both cells should have shown 1.


Reproducible: Always


User Profile Reset: Yes


OpenGL enabled: Yes

Additional Info:
The operation of the function is described in the source file, which I read at this link: https://github.com/LibreOffice/core/blob/14cfff500e93f0d6cbf8412065feea85c01ea81d/scaddins/source/datefunc/datefunc.cxx#L442
Comment 1 Mike Kaganski 2021-11-08 05:14:22 UTC
Repro.
Version: 7.2.2.2 (x64) / LibreOffice Community
Build ID: 02b2acce88a210515b4a5bb2e46cbfb63fe97d56
CPU threads: 12; OS: Windows 10.0 Build 19043; UI render: default; VCL: win
Locale: ru-RU (ru_RU); UI: en-US
Calc: threaded
Comment 2 Mike Kaganski 2021-11-08 06:43:53 UTC
(In reply to Yuri Pieters from comment #0)
> Additional Info:
> The operation of the function is described in the source file, which I read
> at this link:
> https://github.com/LibreOffice/core/blob/
> 14cfff500e93f0d6cbf8412065feea85c01ea81d/scaddins/source/datefunc/datefunc.
> cxx#L442

You are completely correct, and the problem is using integral division by 7 there, which gives 0 for both first week of the year of nStartDate, as well as for the week before it. std::floor should be used instead.

The easy hack should include a unit test.
Comment 3 Mike Kaganski 2021-11-08 07:27:37 UTC
By the way, this is what also gives

  WEEKS(d1;d2;1)+WEEKS(d2;d1;1)=1

for most d1 and d2 when they are in different years.
Comment 4 Winfried Donkers 2021-11-15 10:43:34 UTC
(In reply to Mike Kaganski from comment #3)
> By the way, this is what also gives
> 
>   WEEKS(d1;d2;1)+WEEKS(d2;d1;1)=1
> 
> for most d1 and d2 when they are in different years.

Could you please give one or more numerical examples? I haven't been able to reproduce this incorrect behaviour yet.
Comment 5 Mike Kaganski 2021-11-15 11:03:35 UTC
(In reply to Winfried Donkers (retired) from comment #4)

=WEEKS("1021-01-01";"1020-01-01";1)+WEEKS("1020-01-01";"1021-01-01";1)
Comment 6 Commit Notification 2021-11-29 08:08:52 UTC
Winfried Donkers committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/105196aa00bc0d3e426796f6729a8e7e51271e56

tdf#145578, tdf#145587 revise calculations for WEEKS Add-In function

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Commit Notification 2021-11-30 09:09:38 UTC
Winfried Donkers committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/bd8e584db0c19a30164575d7fc8c583436db5b85

tdf#145578, tdf#145587 revise calculations for WEEKS Add-In function

It will be available in 7.3.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Roman Kuznetsov 2022-05-02 13:18:56 UTC
Winfried, should we close this one as FIXED?
Comment 9 Winfried Donkers 2022-05-02 16:54:14 UTC
(In reply to Roman Kuznetsov from comment #8)
> Winfried, should we close this one as FIXED?

Sorry, I must have forgotten. Fixed now.