classlib: fix SimpleNumber asTimeString for negative values #4802

dyfer · 2020-03-06T01:45:47Z

Purpose and Motivation

Negative SimpleNumber -asTimeString returned incorrect string.

Previously:

-1.asTimeString;
-> 0-1:23:59:59.000

Now:

-1.asTimeString;
-> -00:00:01.000

Types of changes

Bug fix

To-do list

Code is tested
All tests are passing
This PR is ready for review

handle negative values

mossheim

thanks! can you please add the following test cases:

-0.0.asTimeString
-1.asTimeString(dropDaysIfPossible: false)

mossheim · 2020-03-06T03:04:44Z

SCClassLibrary/Common/Math/SimpleNumber.sc

 		decimal = number.asInteger;
 		days = decimal.div(86400).min(maxDays);
 		days = if(dropDaysIfPossible and: { days == 0 }) {
 			days = ""
 		} {
 			days.asString.padLeft(3, "0").add($:);
 		};
+		if(isNegative, {days = "-" ++ days});


in general we use trailing closure for if in the class library. i've added a style point for that here: https://github.com/supercollider/supercollider/wiki/Code-style-guidelines#trailing-closure-syntax

can you change this one here please?

mossheim · 2020-03-06T03:05:13Z

SCClassLibrary/Common/Math/SimpleNumber.sc

+		var number, decimal, days, hours, minutes, seconds, mseconds, isNegative = false;
+		if(this < 0, {
+			isNegative = true;
+		});


this can be simplified to var number, decimal, days, hours, minutes, seconds, mseconds, isNegative = this < 0;

mossheim · 2020-03-06T03:06:36Z

@dyfer gentle reminder to label your PRs!

dyfer · 2020-03-06T03:45:30Z

@dyfer gentle reminder to label your PRs!

Sorry I missed it! And thanks!

dyfer · 2020-03-06T04:10:18Z

Thanks for a quick feedback!

I changed the if statement in the code that I added, but did not touch the one that was there already, should I change that too?

As for

-0.0.asTimeString

I'm not sure what the proper output should be. Intuitively I'd expect

-0.0.asTimeString;
-> -00:00:00.000

but since we don't seem to be able to differentiate between -0.0 and 0.0 (-0.0 == 0.0 -> true), I don't know how to achieve that. Currently this method gives

-0.0.asTimeString;
-> 00:00:00.000

In that case, should this be the expected result of the test?

dyfer · 2020-03-06T04:15:57Z

...I guess I could check for identity:

-0.0 === 0.0
-> false
-0.0 === -0.0
-> true

in the method, as a special case. Is this the way to go?

edit: I added that check, let me know what you think, thanks!

mossheim · 2020-03-06T04:20:28Z

I changed the if statement in the code that I added, but did not touch the one that was there already, should I change that too?

that one seems right already, not sure what you're asking

As for -0.0.asTimeString I'm not sure what the proper output should be

sorry, should have clarified. i think what it does now is fine, and actually makes more sense in my opinion as "-0" is not an intuitively sensible number for most people. this is also a lossy conversion so it's not vital to make sure everything round-trips between this and asSecs perfectly. i just want to pin that behavior so we can be aware if it changes in the future.

dyfer · 2020-03-06T04:23:19Z

that one seems right already, not sure what you're asking

My bad! I was in a rush and misread the code.

sorry, should have clarified. i think what it does now is fine, and actually makes more sense in my opinion as "-0" is not an intuitively sensible number for most people. this is also a lossy conversion so it's not vital to make sure everything round-trips between this and asSecs perfectly. i just want to pin that behavior so we can be aware if it changes in the future.

Sorry, wanted to be done with this and followed that identity path. So now should I revert it and leave the behavior of

-0.0.asTimeString;
-> 00:00:00.000

?

dyfer · 2020-03-06T04:28:19Z

"-00:00:00.000".asSecs;
-> -0.0

so maybe that explicit check for -0.0 should stay?

mossheim · 2020-03-15T16:07:54Z

took some time to think about this one.

i don't think we should output a timestamp saying -0, and the main reason is that while hardware makes an effort to ensure -0 and 0 as floating point values compare equal, there is no such assistance for the strings "-00:00:00" and "00:00:00". by outputting both we now put the burden on the programmer to handle this one case where two strings represent the same time stamp but do not compare equal.

furthermore, the knowledge that the input value that produced "-00:00:00" was -0 or rounded up from slightly below 0 is, in my view, of marginal utility at best, and distracting or confusing at worst.

another reason i prefer to not have a -0 is that fixed-point decimals such as these timestamps are conceptually closer to integers than floating point numbers. importantly, the interval between successive values remains fixed (like integers) rather than increasing exponentially (as with floats).

finally, in my experience the vast number of numerical comparisons are typically done with ==, for good reason -- 0 and -0 wouldn't compare equal under identity ===, but they do under equality ==. and, asTimeString followed by asSecs always produces a float regardless of whether the original number was an integer. for that reason, i think ensuring that a round trip between asTimeString and asSecs preserves identity cannot be a goal here.

so, i think this PR should be modified such that -0.0, -0.0001, etc. all produce the result 00:00:00[.000] with no negative sign. that means checking for negative values after rounding.

dyfer · 2020-03-15T18:31:29Z

Thanks @brianlheim for your thoughts. I changed this PR so it is now

-0.0.asTimeString;
-> 00:00:00.000

that means checking for negative values after rounding.

AFAICT I only need to not check for identity, since

-0.0 < 0;
-> false

I'm just pointing it out to make sure I'm not doing something wrong.

mossheim · 2020-03-15T20:46:19Z

AFAICT I only need to not check for identity,

you also need to handle inputs like -.001 which are negative but may round up to 0 for the requested precision. please make sure to add a test for that.

dyfer · 2020-03-29T01:02:38Z

you also need to handle inputs like -.001 which are negative but may round up to 0 for the requested precision. please make sure to add a test for that.

done!

classlib: fix SimpleNumber asTimeString

4b915f7

handle negative values

dyfer changed the title ~~classlib: fix SimpleNumber asTimeString~~ classlib: fix SimpleNumber asTimeString for negative values Mar 6, 2020

mossheim suggested changes Mar 6, 2020

View reviewed changes

mossheim added the comp: class library SC class library label Mar 6, 2020

dyfer added 2 commits March 5, 2020 20:02

Update SimpleNumber.sc

86587a1

Update TestSimpleNumber.sc

5b7500a

dyfer added 2 commits March 5, 2020 20:19

Update SimpleNumber.sc

a8c4ab1

Update TestSimpleNumber.sc

a779993

dyfer added the bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. label Mar 6, 2020

dyfer added 2 commits March 15, 2020 11:26

Update TestSimpleNumber.sc

14a0b56

Update SimpleNumber.sc

e578ea8

dyfer added 2 commits March 18, 2020 21:25

Update TestSimpleNumber.sc

bceedbb

Update SimpleNumber.sc

f21aac4

mossheim approved these changes Apr 4, 2020

View reviewed changes

mossheim merged commit 901bb06 into supercollider:develop Apr 4, 2020

mossheim mentioned this pull request Apr 11, 2020

cherry-pick 3.11.1 PRs #4865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

classlib: fix SimpleNumber asTimeString for negative values #4802

classlib: fix SimpleNumber asTimeString for negative values #4802

dyfer commented Mar 6, 2020

mossheim left a comment

mossheim Mar 6, 2020

mossheim Mar 6, 2020

mossheim commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020 •

edited

Loading

mossheim commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020

mossheim commented Mar 15, 2020

dyfer commented Mar 15, 2020 •

edited

Loading

mossheim commented Mar 15, 2020

dyfer commented Mar 29, 2020

classlib: fix SimpleNumber asTimeString for negative values #4802

classlib: fix SimpleNumber asTimeString for negative values #4802

Conversation

dyfer commented Mar 6, 2020

Purpose and Motivation

Types of changes

To-do list

mossheim left a comment

Choose a reason for hiding this comment

mossheim Mar 6, 2020

Choose a reason for hiding this comment

mossheim Mar 6, 2020

Choose a reason for hiding this comment

mossheim commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020 • edited Loading

mossheim commented Mar 6, 2020

dyfer commented Mar 6, 2020

dyfer commented Mar 6, 2020

mossheim commented Mar 15, 2020

dyfer commented Mar 15, 2020 • edited Loading

mossheim commented Mar 15, 2020

dyfer commented Mar 29, 2020

dyfer commented Mar 6, 2020 •

edited

Loading

dyfer commented Mar 15, 2020 •

edited

Loading