added benchmarks - #15
Conversation
|
Somehow I missed the notification for this. Thanks for the PR! I'll review it in the next few days. |
|
Ok, thanks for the PR. Looks good in general. I had two thoughts:
|
|
Does this look ok? |
mfogel
left a comment
mfogel
left a comment
There was a problem hiding this comment.
Two typos and a higher-level require()
| asia.features[0].geometry.coordinates, | ||
| [unionPoly.geometry.coordinates]); | ||
| }) | ||
| .add('w83', () => { |
| states.features[0].geometry.coordinates, | ||
| [states.features[1].geometry.coordinates]); | ||
| }) | ||
| .add('w83', () => { |
| const Benchmark = require('benchmark'); | ||
| const jstsUnion = require('@turf/union'); | ||
| const w8r = require('martinez-polygon-clipping'); | ||
| const mfogel = require('../src/index'); |
There was a problem hiding this comment.
I think it would be better if the benchmark went through the api mentioned in the README - which is actually right now in main.js. So this would change to
const mfogel = require('../main');Where mfogel is then used below in the benchmark suites, it would have the exact same form as w8r - ie mfogel.union(...) and the operation require on the next line would not be needed.
|
Thanks for making the directory structure changes, I'll merge this in after the things mentioned in the review I just did are addressed. |
|
Fixed - please let me know if there is anything else. |
|
My only other suggestion here which is super-minor is sometimes people dump the results of the benchmark at the top of the bench.js file so that it is easy to see current results without having to run the benchmark yourself. Obviously you take the results with a grain of salt because it'll depend on what computer they are run on etc, but it does allow a nice quick comprison. |
4 participants
I added the benchmarks from w8r repo plus your implementation. I suspect pointsAsObjects to lower the benchmarks, this could perhaps be done before running the tests.