Topic/simplenumber series

[mtmccrea]

Purpose and Motivation

This fixes #4452, where SimpleNumber:series can overshoot its last value if the step is specified as a float and does not evenly divide into the range of the series.

The problem is illustrated here:

(0, 39952.0 .. 958845).last
// -> 958848.0 // overshoots 'last' value
(0, 39952   .. 958845).last
// -> 918896   // int type stops short of 'last', as expected
(0, 2.00001 .. 8).last
// -> 8.0004   // overshoots when step is near an even division
(0, 3.0 .. 8.0).last
// -> 6.0      // otherwise stops short

Types of changes

• Bug fix

To-do list

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

[mtmccrea]

Looks like one of the travis builds failed:

C++ QT=false DO_LINT=true 44 sec linux

with this error:

clang-format found, but incorrect version at clang-format with version: clang-format version 5.0.0 (tags/RELEASE_500/final)`

Is this a problem with the CI build configuration? Looks like the clang-format version needs to be specified to be 8.0.0

EDIT: I triggered another build and it looks like it went OK... hmmmm.

[mossheim]

@supercollider/reviewers Anyone want to take this one? :)

[lnihlen]

You're right to catch that this line creates a bug, but I also worry that it prevents a different bug. Is it now possible to create a divide-by-zero error here with a zero value for step? I tested this patch with a step=0 and I know the error check on line 867 is catching this, but would it make sense to add a special check for a zero step size and report that error before attempting the divide?

[lnihlen]

Hey @mtmccrea thanks for the PR! I had one question but overall looks good.

[mtmccrea]

Thanks for having a look at this @lnihlen!

Good catch. I tested the 0-step case and it was caught as you saw, but it's a good point about the divide by zero.

Out of curiosity, I inserted some checks to see what the math was doing:

        step = second - first;
		post(
			 "prSimpleNumberSeries: first: %f, step: %f, last %f\n",
			 first, step, last);
        size = (int)((last - first) / step) + 1;
		post(
			 "prSimpleNumberSeries: size: %d\n", size);
		
        if ((size < 1) || ((step >= 0) && (last < first)) || ((step <= 0) && (last > first))) {
            post(
                "prSimpleNumberSeries: arguments do not form an arithmetic progression: first: %f, step: %f, last %f\n",
                first, step, last);
            return errFailed;
        }

and with this input:
(1.2, 1.2 .. 100)
here's what went through:

prSimpleNumberSeries: first: 1.200000, step: 0.000000, last 100.000000
prSimpleNumberSeries: size: -2147483647
prSimpleNumberSeries: arguments do not form an arithmetic progression: 
first: 1.200000, step: 0.000000, last 100.000000

So yes, worth catching.
I found that there was an inconsistency between Int and Float behavior:

/* 0-step, first == last */
1.series(1, 1)
// -> [ 1 ]
1.2.series(1.2, 1.2)
// -> ERROR: Primitive '_SimpleNumberSeries' failed.

So rather than error out on a step of 0, I now catch the case and set size to 1. So the behavior is now consistent with Int:

/* 0-step, first == last */
1.series(1, 1)
// -> [ 1 ]
1.2.series(1.2, 1.2)
// -> [ 1.2 ]

I'll push this change shortly.
Thanks!

[mtmccrea]

I've also added a unit test for this case.

[mtmccrea]

OK, this is ready for review!

[mtmccrea]

I've also added a unit test for this case.

[lnihlen]

Sweet, thanks for the great follow-through @mtmccrea! Looks like CI is reporting a C++ linter error on the change to the error reporting string, but otherwise this is LGTM.

[mtmccrea]

Working through some clang-format issues now...

[mtmccrea]

OK, so it looks like appveyor travis CI is getting caught up on linting issues on lines that the clang-format.py script itself modified (4134851).
Does that mean there's either a drift in the rules between my clang-format, and the one installed on the ci machine?
Perhaps @brianlheim has seen an issue like this?

[lnihlen]

Which version of clang-format are you using? We are pinned on 8.0.0 right now.

[mtmccrea]

-> clang-format --version
clang-format version 8.0.0 (tags/RELEASE_800/final)

I just installed it again from the LLVM site.

[mtmccrea]

FWIW the edit that clang-format made appears to be related to capping the line length, then wrapping the remainder to the next line, and indenting it to align with the open parenthesis of the post( method above it.

[mtmccrea]

I think I may have found the problem. We'll see what CI says...

[mtmccrea]

OK CI is passing.

This may be a missed case in clang-format. It was actually a couple lines above the auto-format change I mentioned before. There was a line with auto-indent spaces at the beginning but the line was otherwise empty. Running:

-> python3 ../tools/clang-format.py lint
--- lang/LangPrimSource/PyrMathPrim.cpp	(before formatting)
+++ lang/LangPrimSource/PyrMathPrim.cpp	(after formatting)
@@ -867,7 +867,7 @@
         } else {
             size = (int)((last - first) / step) + 1;
         }
-        // << THERE ARE SPACES HERE
+
         if ((size < 1) || ((step >= 0) && (last < first)) || ((step <= 0) && (last > first))) {
             post("prSimpleNumberSeries: arguments do not form an arithmetic progression:\n\tfirst: %f, step: %f, last "
                  "%f\n",

Yet running:

-> python3 ../tools/clang-format.py format

does not remove these spaces. So it's caught when CI lints the file.

I can file this as a bug if it's agreed this shouldn't happen.

[lnihlen]

I think a bug would be helpful, thanks! And thanks for taking a look into the CI issue. As you can see we're still working out some of the kinks in the new tooling. This PR looks ready for merge to me.

[mtmccrea]

Thanks, @lnihlen! I've filed the format issue: #4464.