Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make initial option in map/reduce optional #114

Merged
merged 1 commit into from
Jan 16, 2019
Merged

Make initial option in map/reduce optional #114

merged 1 commit into from
Jan 16, 2019

Conversation

mourner
Copy link
Member

@mourner mourner commented Jan 15, 2019

This makes map/reduce more convenient to use by making it possible to omit the initial function in most cases.

When initial is omitted, the reducer will use the mapped values of the first reduced item as the initial value. So trivial cases like +, min and max will work seamlessly without the need for initial.

It's kind of breaking, but I doubt many people depended on initial returning {} by default, so maybe it's OK to release this as a minor semver update.

cc @redbmk

@mourner
Copy link
Member Author

mourner commented Jan 15, 2019

Thinking on this further, maybe we should ditch the initial option for good. I can't come up with a good use case that would justify it, given that reduction can't happen on an empty list, and there's a map function for coercing types.

@redbmk
Copy link
Collaborator

redbmk commented Jan 16, 2019

Yeah, this does sound like a breaking change... Maybe there's a case where someone is checking for undefined in the reducer, or maybe providing something to initial that doesn't exist in any of the mapped values.

I can't think of a concrete example off the top of my head. The code looks like it would be a bit cleaner without initial though. I'm looking through some things I've used supercluster for in the past to see if I can come up with a counter-example where initial would be required.

@redbmk
Copy link
Collaborator

redbmk commented Jan 16, 2019

Alright, I did find a real-world example of using initial in a way that would be more difficult, but maybe still possible, if it were just using the first value from a map.

In this dataset, items had a quality (which could be undefined) and the style would display a different color depending on the quality. Items with a poor or undefined quality wouldn't be shown, but clusters would always be shown even if it was full of poor quality items (the same style was used for clusters as well as single items).

The code ended up looking something like this:

const ranks = {
  great: 1,
  good: 2,
  alright: 3,
  clustered: 4,
  bad: 5,
  terrible: 6
};

SuperCluster({
  initial: () => ({ quality: 'clustered' }),
  map: ({ quality = 'clustered' }) => ({ quality }),
  reduce: (cluster, { quality }) => {
    if (ranks[quality] < ranks[cluster.quality]) cluster.quality = quality;
  },
})

I think it would be possible to tweak the stylesheet to just get the best rank within the points, and then if it's worse than alright, but still had a point_count, then we would know it's a cluster and could show it.

In this case, removing the option for initial would definitely break this code, though a workaround is probably doable. I'm not sure if there would be other, similar cases where you couldn't just adjust the stylesheet (assuming you're using supercluster along with mapbox and not for some other purpose).

@mourner
Copy link
Member Author

mourner commented Jan 16, 2019

@redbmk thangs for digging into this! The example is pretty confusing, but as far as I understand, in this case, the difference would essentially be:

clusterQuality = bestRank('clustered', quality1, quality2, ...) // with initial
clusterQuality = bestRank(quality1, quality2, ...) // without initial

The second seems more flexible to me because it retains more information (and it's better to tweak the final filter IMO) . But if you want the same behavior as previously, you'd just set up map in a way that doesn't output rank lower than 'clustered'.

It seems to me that removing initial could help people design map/reduce so that it's clearer in intent, even if sometimes more verbose. I still can't think of cases where one would absolutely need the initial without a potential workaround. So I'm still inclined towards removing it because at this point we could do it without much damage — e.g. do a major semver bump, documenting the breaking change, and then quickly change the corresponding Mapbox GL JS style spec design before it got to its first release.

@mourner
Copy link
Member Author

mourner commented Jan 16, 2019

Going to merge this as is (making initial optional but still available) because it's an improvement regardless of what we decide about the style spec syntax, and aligns better with e.g. how JavaScript reduce works. Still keen on discussing the style spec thing though.

@mourner mourner merged commit 2b0e89b into master Jan 16, 2019
@mourner mourner deleted the no-initial branch January 16, 2019 18:19
@redbmk
Copy link
Collaborator

redbmk commented Jan 16, 2019

Ah, yeah actually what you said about the map function not returning anything less than clustered makes a lot of sense. I think without initial at worst you would just need to be a little more clever to implement something.

To nitpick, JavaScript reduce actually does allow for an optional initial value. But regardless, I don't know that it's actually necessary in this case. If there are no points, then there will be no cluster, so no need for an initial value.

@mourner
Copy link
Member Author

mourner commented Jan 16, 2019

@redbmk there's some more context on initial value use in this Wikipedia article, and as far as I understand, it mostly comes down to 1) supporting the use case of resulting value of a different type than the items, and 2) not throwing an error on empty arrays. For type coercion, we have the map expression, and empty arrays never happen because we need at least 2 points to form a cluster, so I think not offering initial value option is OK. (BTW, CouchDB views are an example of another map/reduce API where there's no concept of an initial value).

@redbmk
Copy link
Collaborator

redbmk commented Jan 16, 2019

Yeah, sounds good to me. The more I think about it, the less I think we need initial. I vote for removing it altogether and having a major version bump.

mourner added a commit that referenced this pull request Jan 17, 2019
See discussion in #114 for context.
mourner added a commit that referenced this pull request Jan 17, 2019
See discussion in #114 for context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants