Open farkmarnum opened 3 weeks ago
Hi @farkmarnum it's unfortunate a change was not documented. Can you confirm what the new version 7.0 turf.mask behavior is? .The simplest approach is probably just to document the existing behavior in the JSDoc comments for the function (ends up published in the API docs). If you do put together a code PR as described, I think it would be welcome, no reservation here. And I would be curious the difference.
@twelch just checked and the behavior on 7.0.0
is the same (the mask
input is mutated).
I've opened a PR to fix here: #2635
@twelch and @rowanwins do we know if the mutating behaviour introduced in 6.5.0 was intentional? The gist of that PR seems to be performance related, so maybe it wasn't?
I'm open to hearing straight from Rowan. What I've read is that mutating was not part of the original design/spirit and later became opt-in for a few functions. I'm not sure if that's held true since -- https://github.com/Turfjs/turf/issues/693
Some additional thoughts. Whether intentional or not, the precedent in the code seems to be mutation as an opt-in option, not a default behavior. At first glance I see the mutate
option available (for performance reasons) for truncate, simplify, transformScale, transformRotate, rewind, projection, turf-line-to-polygon, flip, polygon-dissolve, cluster-kmeans, dissolve, concave.
I noticed that in 6.2.0 line-to-polygon was changed to no longer mutate by default, but rather use the option.
I just updated the benchmark code in that PR as well. Interestingly, there doesn't seem to be much of a performance difference, just a small perf boost when mutate = true
:
basic (mutate = false) x 294,373 ops/sec ±0.25% (95 runs sampled)
basic (mutate = true) x 307,397 ops/sec ±0.13% (97 runs sampled)
mask-outside (mutate = false) x 100,575 ops/sec ±0.55% (97 runs sampled)
mask-outside (mutate = true) x 103,180 ops/sec ±0.40% (94 runs sampled)
Agree mutation should be opt in. If unintentional, we can fix straight away as a bug. If it was intentional, and just not documented, that makes it a bit murkier whether it's a breaking change.
Let's see how good @rowanwins memory is!
Haven't heard any further on this. From looking at the tests, am guessing the new behaviour wasn't intentional.
Will do a comparison with a large file to see which works best - ~6.5.0~ 6.3.0 code as it was, or current code with a clone by default. From there we can decide whether to roll back or push ahead with this PR. How does that sound @farkmarnum and @twelch?
On version
v6.5.0
,turf.mask(polygon, mask)
now mutatesmask
in place. In versionv6.3.0
, it did not.Since this change is not documented, it can lead to some hard-to-diagnose bugs.
Here's a simple test to verify:
This is due to the changes in #2130, which rewrote
turf.mask()
.The simplest solution is probably to add a
mutate
option and default it tofalse
(liketurf-simplify
does). I'll make a PR with that approach.