Closed Sophist-UK closed 2 years ago
I'm not convinced this is much if any of a subpath improvement, afterall the subpath class itself is just a window that stores the start and end points of that subpath. yield self.subpath(i, subpath_map=subpath_map)
doesn't seem like it would be any faster than just calculating the subpath elements. A list of Subpath() objects that return instantly doesn't seem slower than a list of integers for subpath divisions, especially considering Subpath() objects are just slightly fancier lists of integers anyways.
Also, the use of "subpath_map" strongly implies a dict() like hashmap but, it's just a regular list. And you are required to fully solve the sublists to use them. Whereas as_subpath() could have done a break
somewhere in the subpath calculations, since it was using a generator. You might be able to do a generator in _subpath_map()
but you still would end up doing the list(_subpath_map())
to count them which wouldn't be any speed advantage at all.
Also I fear this is hyper-optimizing nothing since this is literally the same time complexity. The Subpath class is extremely lightweight, and isn't any more harder than storing values in a list.
I did test the performance. as_subpath
is a bit faster, count_subpaths
is about twice as fast, subpath
also about twice as fast for a first call, substantially faster if subpath_map is pre-calculated.
_subpath_map
was a name chosen when I started to develop it - and I was using map
to build a bitmap for Move
instances. But it could be called _subpath_indices
if you wish.
A list of Subpath() objects that return instantly doesn't seem slower than a list of integers for subpath divisions, especially considering Subpath() objects are just slightly fancier lists of integers anyways
a. It's an iterator - so Pythonesque. b. It's backwards compatible.
I'll get around to running some final checks on the code. Seems like _subpath_indices might still be better as an generator, and I doubt anybody is going to be using this code enough to save the pre-calculated values.
That is why the method is _subpath_indices
and not subpath_indices
😃
Seems like _subpath_indices might still be better as an generator...
For as_subpaths
a generator would make sense. But for subpath
it doesn't. So if we wanted to use a generator for as_subpaths
then we would need to code it differently so it doesn't call subpath
. I will think about this.
Ok - I have reimplemented as a generator. Here are the test results:
m = Path("M0,0 0,1 1,1 1,0 z M2,2 2,3 3,3 3,2 z M4,4 4,5 5,5 5,4 z M6,6 6,7 7,7 7,6 z M8,8 8,9 9,9 9,8 z M10,10 10,11 11,11 11,10 z M12,12 12,13 13,13 13,12 z M14,14 14,15 15,15 15,14 z M16,16 16,17 17,17 17,16 z M18,18 18,19 19,19 19,18 z")
import timeit
print("indices",timeit.timeit(lambda: tuple(m._subpath_indices()), number=100000))
print("old count",timeit.timeit(lambda: m.old_count_subpaths(), number=100000))
print("new count",timeit.timeit(lambda: m.count_subpaths(), number=100000))
print("old subpath",timeit.timeit(lambda: m.old_subpath(5), number=100000))
print("new subpath",timeit.timeit(lambda: m.subpath(5), number=100000))
print("old as_subpaths once",timeit.timeit(lambda: m.old_as_subpaths(), number=100000))
print("new as_subpaths once",timeit.timeit(lambda: m.as_subpaths(), number=100000))
print("old as_subpaths all",timeit.timeit(lambda: [s for s in m.old_as_subpaths()], number=100000))
print("new as_subpaths all",timeit.timeit(lambda: [s for s in m.as_subpaths()], number=100000))
indices 1.4540482000000003
old count 2.7409119000000004
new count 1.4271927
old subpath 2.6614028999999997
new subpath 1.0155873
old as_subpaths once 0.0357125000000007
new as_subpaths once 0.03644350000000074
old as_subpaths all 2.5767454999999995
new as_subpaths all 2.0661541999999997
Yeah, I think this code bit is solid. Though the subpath code might be wrong considering Z as subpath separator within the spec.
I'll mull this over a bit more before merging, still needs some extended checks.
I'll fix the Z issue and add appropriate tests before merge.
This already handles a close without a Move afterwards. IMO it is ready to merge.
I should point out that svgelements has 3 other instances of raise StopIteration
. I do not propose to change these as part of this PR, however they should presumably be addressed.
On reflection, since raise StopIteration
should be replaced by return
I see no reason why I cannot fix this in the same PR as it would seem to be low risk.
Bump
Bump
Manually reverted in 1.6.12 to rectify bug caused by this code.
MeerK40t crash log. Version: 0.8.0003 RC2 on Windows:AMD64 - 4.1.1 msw (phoenix) wxWidgets 3.1.5
Traceback (most recent call last):
File "meerk40t\gui\laserpanel.py", line 326, in on_button_update
File "meerk40t\kernel\context.py", line 33, in __call__
File "meerk40t\kernel\kernel.py", line 2004, in console
File "meerk40t\kernel\kernel.py", line 2089, in _console_parse
File "meerk40t\kernel\functions.py", line 215, in inner
File "meerk40t\core\planner.py", line 640, in plan_blob
File "meerk40t\core\cutplan.py", line 194, in blob
File "meerk40t\core\cutcode.py", line 340, in __init__
File "meerk40t\core\cutcode.py", line 224, in __init__
File "meerk40t\core\node\laserop.py", line 507, in as_cutobjects
File "meerk40t\svgelements.py", line 6089, in as_subpaths
UnboundLocalError: local variable 'end' referenced before assignment
Improve speed of
count_subpaths
,subpath
andas_subpaths
.