Add documentation and unittests regarding multisets#5643
Add documentation and unittests regarding multisets#5643wilzbach wants to merge 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request, @wilzbach! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
wilzbach
requested review from
JackStouffer,
PetarKirov and
andralex
as code owners
| All the algorithms, except (LREF multiwayMerge) and (LREF mutiwayUnion), | ||
| are generalized to accept as input not only sets but also multisets: | ||
| $(UL | ||
| $(UI when all ranges passed as input are sets, the result is going to be |
There was a problem hiding this comment.
/dev/shm/dtest/work/repo/dlang.org/web/phobos-prerelease/std_algorithm_setops.html:157:<ul> <!--UNDEFINED MACRO: "UI"-->
/dev/shm/dtest/work/repo/dlang.org/web/phobos-prerelease/std_algorithm_setops.html:158: <!--UNDEFINED MACRO: "UI"-->
@RazvanN7 as we currently can't rebase PRs without breaking GH hooks, could you please fix this on your branch (should be LI) and resubmit a PR? Thanks!
| $(LREF setIntersection), $(LREF setSymmetricDifference) expect a range of sorted | ||
| ranges as input. | ||
|
|
||
| All the algorithms, except (LREF multiwayMerge) and (LREF mutiwayUnion), |
There was a problem hiding this comment.
s/All the/All/
Since you say "all except" it's better to simple enumerate the algorithms you refer to: "The algorithms
| ranges as input. | ||
|
|
||
| All the algorithms, except (LREF multiwayMerge) and (LREF mutiwayUnion), | ||
| are generalized to accept as input not only sets but also multisets: |
There was a problem hiding this comment.
Hotlink the word "multisets" to the Wikipedia article.
| $(UI when all ranges passed as input are sets, the result is going to be | ||
| a set.) | ||
| $(UI if one of the ranges is a multiset, the result is going to be a multiset.) | ||
| ) |
There was a problem hiding this comment.
This is tenuous. For example, intersecting the multiset {a, a, b} with the multiset {b, c, c} is {b} i.e. a set and not a multiset as the text above implies.
Instead of using imprecise text to then explain it, just say what happens:
"... but also multisets. Each algorithm documents behavior in the presence of duplicated inputs."
| [4, 7], | ||
| [7] | ||
| ]; | ||
| auto y = new Tuple!(double, uint)[2]; |
There was a problem hiding this comment.
why not auto? the rest of the examples are like this.
There was a problem hiding this comment.
This works: int[10] a;
This doesn't: auto a = int[10];
There was a problem hiding this comment.
note that crucially the statically sized array avoids memory allocation
| $(T2 multiwayMerge, | ||
| Computes the union of a set of sets implemented as a range of sorted | ||
| ranges.) | ||
| Computes the merging of a range of sorted ranges.) |
There was a problem hiding this comment.
"Merging" is not a noun; it's a gerund. Therefore "computes the merging" is incorrect. The noun that would fit there is "mergence" (https://www.merriam-webster.com/dictionary/mergence) but that is used so infrequently it sure is liable to cause confusion. "Merger" is also a good fit but it's seldom used in algorithmic contexts.
Just rephrase "Computes the merge" as e.g. "Merges" everywhere.
| // MultiwayMerge | ||
| /** | ||
| Computes the union of multiple sets. The input sets are passed as a | ||
| Computes the merging of multiple sets. The input sets are passed as a |
| `multiwayUnion(ror)` is functionally equivalent to `multiwayMerge(ror).uniq`. | ||
|
|
||
| `multiwayUnion` returns a set regardless of the type of the input ranges | ||
| (sets or multisets). |
There was a problem hiding this comment.
We don't distinguish sets/multisets by "type" so the sentence is misleading. Replace with:
"The output of multiwayUnion has no duplicates even when its inputs contain duplicates."
|
|
||
| If all input ranges are sets (i.e. without duplicated elements), | ||
| the resulting range is going to be a set, otherwise the result | ||
| is going to be a multiset. In the case of multisets, the range with |
There was a problem hiding this comment.
No, as shown above intersecting multisets may produce a set. Just delete the first sentence in this paragraph; the second contains all needed information.
|
Close this |
4 participants
Resubmission of #5627 due to GH's annoying hook issue (they send the wrong sha after a rebase).