New API for calcExtents (Issue #44)

[jtrim-ons]

Issue #44

This PR changes the API of calcExtents to use an object rather than an array of accessor functions. It also fixes a bug where the data passed could be modified (in rare circumstances).

I have made a number of changes to be behavior. Most of these are described in this comment. In addition:

• The function no longer treats data.length==0 as a special case; it just returns [null,null] for each extent.
• The function throws a TypeError if the parameters are not an array and an object.

I'm not confident that all of these are good decisions, but hopefully the code is helpful in some way!

[mhkeller]

Sorry for the delay on this. I made some minor tweaks that I'll go over and let me know your thoughts. I pushed to this branch but if you give me push access to your forked repo I can add those commits onto this PR to keep it all nice and organized. Let me know what you think on any and all!

1. I converted it from a for..in loop to an old-fashioned three-part for-loop since the linter was complaining about it. Apparently it doesn't like those? I'm pretty agnostic but this should be equivalent
2. I moved the min and max function into their own files and then, since they were 99 percent the same, I made them one function with a currying argument.
3. I wasn't super used to using continue statements and so I converted the if-statement so it would work without that. I changed the != null check to a !== null and added an explicit check for undefined just to make it more explicit about what it's checking for. But if there are other reasons or cases that the !=null catches then we should add those.
4. Added tests for minMax
5. Updated LayerCake.svetle to use the new API

Thanks again for making so much progress on this and sorry for the delay in reviewing it – I've had a busy few weeks.

[mhkeller]

Looks like if you do this, then I can push directly to your branch

If you get an error when pushing, the other user might have unchecked the box in the pull request that says “Allow edits from maintainers.” Have them check that box in the pull request view and try again.

[mhkeller]

6. I made it so that instead of it returning {} if fields is undefined, it throws an error.

[jtrim-ons]

Those changes all sound good to me. I'll take a look at the code in a moment. Particularly the continue thing - I was a bit unhappy with how I'd written that part!

It's strange - I hadn't unchecked "Allow edits from maintainers". I've tried unchecking and re-checking it to see if that helps, but I guess it won't. I've also sent you an invitation to edit my fork.

[jtrim-ons]

I've had a read through your edits, but I haven't tried downloading and running them. They all look great to me.

A couple of thoughts, probably not important:

• result = result === null || val > result ? val : result; and the similar line for <: would it be nicer to write if (result === null || val > result) { result = val; }? (I think I wrote the original version of these lines, so I'm changing my mind here!)
• Have you tested whether calcExtents still runs fast enough? I wonder if it might be worth profiling with a large dataset to make sure. I might have time next week if you'd like me to give it a quick check - let me know. I wonder if the curried minMax might have a slight performance cost. I guess the cost is probably negligible, and I like the way you've written it.
• I tried to be very careful in the code I wrote, but I'm a bit nervous about introducing bugs, so please do double-check my work!

[mhkeller]

I'll go over it closely and double check it. I haven't done any benchmarking but if you had some time to put something together comparing the new vs the old version that would be great. What's the performance hit for using currying? That result ternary was from your version but I'll take a look at that conditional and see what could work.

[mhkeller]

Cool that worked and I pushed those commits to this branch

[jtrim-ons]

I don't know the proper way to do benchmarking, but I tried running this script to get an idea of how slow or fast calcExtents is:

import calcExtents from './src/lib/calcExtents.js';

const x = {args: [[], {z:d=>d}], expected: {z: [0, 5]}};
for (let i=0; i<500000; i++) {
    x.args[0].push(0);
    x.args[0].push(5);
}

for (let i=0; i<1000; i++) {
  console.log(calcExtents(...x.args));
}
console.log(x.expected);

(It creates an array with a million elements, then runs calcExtents 1000 times).

The new version takes 15.6 seconds on my laptop, the non-curried version is about the same, and the pre-pull-request version takes only 5.55 seconds. So, it's about 15 ms vs 5 ms for each traversal over the million elements (although perhaps the first run of calcExtents is slower, given how JavaScript engines work???)

My feeling is that the slowdown is worth it to have slightly more readable code, since 1000000-element arrays are uncommon. What do you think? I can inline the min and max functions again if you think the slowdown is a problem.

[jtrim-ons]

I tried running calcExtents just once instead of 1000 times. It looks like it takes about 0.02 seconds with the latest code.

[mhkeller]

Interesting! I'll take a look and see where the differences occur I'm kinda curious.

[mhkeller]

I put together three versions on this branch and compared them to the original.

original: 0.245ms
jtrim: 50.069ms
new: 13.97ms
new-1: 18.26ms

I'm really curious why the new api takes so long. maybe the Object.keys call is expensive.

[mhkeller]

Nevermind, I was doing something stupid. These are the results:

new: 14.092ms
original: 17.563ms
jtrim: 52.683ms
new-1: 19.641ms

I included the news test for modifying data and it passes – but I didn't really change that much from the original calcExtents.js function. @jtrim-ons could you take a look and see if actually does fix that problem? I'm a bit inclined to go with the faster option just cause I'd not want to be the slow part of someone's app but open to ways to improve things!

[jtrim-ons]

I totally agree with your changes. Sorry that the min/max functions were a bit of a waste of time.

I think your current version of the function is great, and as far as I can tell it's correct.

My only tiny further suggestion is in the following diff:

It's possibly a tiny bit more readable? It also seems to be marginally faster (newX is the modified version):

new: 12.466ms
newX: 10.753ms
original: 14.427ms
jtrim: 18.876ms
new-1: 17.93ms

But I think the code is great as it is and there's no need to make this further change unless you really feel like it!

[jtrim-ons]

PS it's interesting that jtrim isn't so bad on my machine, but given the difference on yours I think it's better to use the faster version.

[mhkeller]

Cool thanks for looking at it! Yes I like that min max variable idea. I'll push the new version to this branch and you can add that.

[jtrim-ons]

Done :-)

[mhkeller]

Thanks! I'll prep the examples and everything and merge this in. I'll do some additional testing on the existing examples too, just in case.