Error in rule_elliptical_arc crashes scour

To fix, need to replace "< 0.0" with "< 0" , because Decimal('1.5') < 0.0 = True

Hello,

Can you tell me which version of scour you are using and what command (options) you used? I used the latest scour script from the bzr repo and ran scour with default options and did not see any crashes.

Thanks!
Jeff

Please add to the above information your version of Python, which is output by 'python -V'.

Like codedread, I see nothing wrong with default options using revision 193 from Bazaar. I'm running Python 2.6.5.

I have the same issue.

./scour.py -i 2.svg -o out.svg
scour 0.25
Copyright Jeff Schiller, Louis Simard, 2010
Traceback (most recent call last):
  File "./scour.py", line 2926, in <module>
    out_string = scourString(in_string, options).encode("UTF-8")
  File "./scour.py", line 2718, in scourString
    cleanPath(elem, options)
  File "./scour.py", line 1556, in cleanPath
    path = svg_parser.parse(oldPathStr)
  File "/home/wallex/scripts/scour/svg_regex.py", line 150, in parse
    return self.rule_svg_path(next, token)
  File "/home/wallex/scripts/scour/svg_regex.py", line 158, in rule_svg_path
    command_group, token = rule(next, token)
  File "/home/wallex/scripts/scour/svg_regex.py", line 225, in rule_elliptical_arc
    raise SyntaxError("expecting a nonnegative number; got %r" % (token,))
SyntaxError: expecting a nonnegative number; got ('float', u'2.53433')

> python -V
Python 2.6.5

Could this be some kind of python configuration issue?
See the attached python test script, in my machine it triggers the same kind of error (Decimal("0.234") < 0.0 true).

 ./test.py
Traceback (most recent call last):
  File "./test.py", line 8, in <module>
    raise SyntaxError("%r expecting a nonnegative number; got %r" % (rx,token,))
SyntaxError: Decimal('0.234') expecting a nonnegative number; got '0.234'

Hmm, I found this bug report about it:

http://bugs.python.org/issue2531

If I switch to Python 3.1.2, running the test file gives me :

 ./test.py
Traceback (most recent call last):
  File "./test.py", line 7, in <module>
    if rx <0.0:
TypeError: unorderable types: Decimal() < float()

I suggest the fix should be

ry = Decimal(token[1]) * 1
if ry < Decimal("0.0"):

This makes the test file work correctly for me again.

Thanks for investigating this bug.

Despite being unable to reproduce it myself (still), I have applied your proposed fix to the trunk in order to fix it.

That is strange. Maybe the configuration has to do with it?

I wonder if the error is really that the *1 returns a Decimal object in my system (and that of the bug reporter), but it returns a Float in your system (hence why the bug doesn't trigger)? If so, what are the implications down the line? Perhaps the proper bug-fix isn't changing the check, but ensuring that rx and ry are Floats rather than Decimal objects.

If you want is to use Floats around the code, then the conversion code perhaps shouldn't be "rx = Decimal(token[i]) * 1" but a "rx = float(token[i])"
http://docs.python.org/library/functions.html#float

Checking around, scour uses the Decimal()*1 line in nine places:

svg_regex.py:223: rx = Decimal(token[1]) * 1
svg_regex.py:230: ry = Decimal(token[1]) * 1
svg_regex.py:237: axis_rotation = Decimal(token[1]) * 1
svg_regex.py:242: large_arc_flag = Decimal(token[1]) * 1
svg_regex.py:247: sweep_flag = Decimal(token[1]) * 1
svg_regex.py:252: x = Decimal(token[1]) * 1
svg_regex.py:257: y = Decimal(token[1]) * 1
svg_transform.py:220: x = Decimal(token[1]) * 1
svg_transform.py:228: x = Decimal(token[1]) * 1

But these other lines make me wonder if you really wanted Decimal objects? Maybe they just are a different use-case.

scour.py:59:# Python 2.3- did not have Decimal
scour.py:2099: if not isinstance(length, Decimal):
scour.py:2131: and reduces their precision to the current Decimal context's precision.
scour.py:2207: Decimal(math.degrees(math.asin(transform[0][1][4])))

Anyway, I don't think using "*1" is intuitive in any way regarding it's purpose....

We do want to use Decimal, for the --precision option.

From the Python docs [1]:

> The solution [to round the inputs to an arithmetic operation]
> is either to increase precision or to force rounding of inputs
> using the unary plus operation:

> >>> getcontext().prec = 3
> >>> +Decimal('1.23456789') # unary plus triggers rounding
> Decimal('1.23')

Decimal * 1 does like +Decimal, making sure that all coordinates are rounded to the precision specified on the command line before any operation is carried out on them. Otherwise, coordinates that have no operations carried out on them would be kept in their full precisions in the resulting file, despite the --precision option. This is a way to force at least one operation to be carried out on the numbers.

Also, if we were to use floats, coordinates could become monstrosities such as 3.240000001, which are definitely not an optimisation :)

As for * 1, there's another thing I used in svg_regex.py to create Decimal objects:

svg_regex.py:276: x = getcontext().create_decimal(token[1])
svg_regex.py:280: y = getcontext().create_decimal(token[1])

Maybe that would be more intuitive?

Hmm, even if more intuitive, it looks a little uglier to read. Though I wonder why "Decimal()*1" was picked over "+Decimal()". The unary plus looks mysterious, but (IMHO) less confusion-prone than *1

Anyway, I am just not an experienced Python developer, so that may in part explain why it looks strange to me.

Another possibility to check for negativity (which should be faster than rx < Decimal("0.0"):) could be:

if rx.is_signed():

The only difference between a signed check and a < 0.0 check is that Decimal("-0.0").is_signed() returns true (but then again -0 is mathematically different from +0, no?)

http://docs.python.org/library/decimal.html#decimal.Decimal.is_signed

Decimal.is_signed() is new in Python 2.6. This code is intended to run on Python 2.4 and later.

This bug was fixed in the package scour - 0.25+bzr207-1

---------------
scour (0.25+bzr207-1) unstable; urgency=low

  * New upstream bzr snapshot:
    - Add the option --no-renderer-workaround.
    - Fix gradient corruption. (LP: #702423)
    - Add option to only remove autogenerated id's. (LP: #492277)
    - Fix Windows line ending handling. (LP: #717826)
    - Fix occasional production of empty defs. (LP: #717254)
    - Remove more attributes with default values. (LP: #714731)
    - Fix wrong handling of file:// references. (LP: #708515)
    - Delete text attributes from groups with only non-text elements.
      (LP: #714727)
    - Remove unnecessary text-align properties from non-text elements.
      (LP: #714720)
    - Fix wrong optimization of 0-length Bézier curves. (LP: #714717)
    - Fix decimal number processing in rule_elliptical_arc(). (LP: #638764)
    - Fix transform matrix order. (LP: #722544)
  * Drop 01-bzr195.patch, upstream now.
  * debian/cmpsvg: Fix computation of difference to add up the absolute
    difference of each pixel.
  * debian/cmpsvg: Show percentages with three digits after comma.
  * debian/cmpsvg: Lower default treshold to 0.05%.
 -- Martin Pitt <email address hidden> Tue, 15 Mar 2011 09:28:45 +0100

Fix committed in the trunk, revision 10180.

This bug is fixed in release 0.26 of Scour.