Fix issue 355 added get frequency getter to iterable #357

pvlKryu · 2024-08-02T17:10:28Z

[yes] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Description:
This Pull Request introduces a new extension getter frequencies for the Iterable class. The countFrequency method returns a map where the keys are the unique elements of the iterable and the values are the counts of those elements. This addition provides an easy and efficient way to count the frequency of elements in any iterable.

Changes:

Added frequencies getter extension to lib/src/iterable_extensions.dart
Implemented comprehensive tests for the frequencies getter in test/extensions_test.dart. The tests cover various subclasses of Iterable including List, Set, and Queue.

Relevant Issues:
This PR addresses Issue dart-lang/core#679.

Thank you!

lib/src/iterable_extensions.dart

Co-authored-by: Mateus Felipe C. C. Pinto <[email protected]>

pvlKryu · 2024-09-03T06:53:05Z

@mateusfccp Hey Mateus, can you approve?

pvlKryu · 2024-09-03T06:54:58Z

@kevmoo can you please review this one?

mateusfccp · 2024-09-03T12:29:55Z

@mateusfccp Hey Mateus, can you approve?

I can't, I'm not a member of the team.

lrhn · 2024-09-13T22:20:29Z

lib/src/iterable_extensions.dart

+  ///
+  /// For example, `['a', 'b', 'b', 'c', 'c', 'c'].countFrequency()`
+  /// returns `{'a': 1, 'b': 2, 'c': 3}`.
+  Map<T, int> countFrequency() {


Would probably make it get frequencies instead of a function.

thank you, I agree this way is better, please check my changes

lrhn · 2024-09-13T22:22:06Z

lib/src/iterable_extensions.dart

+  Map<T, int> countFrequency() {
+    var frequencyMap = <T, int>{};
+    for (var item in this) {
+      frequencyMap[item] = (frequencyMap[item] ?? 0) + 1;


Can use update, which avoids doing two lookups per item.

frequencyMap.update(item, _increment, ifAbsent: _one);

with top-level/static helpers:

int _increment(int value) => value + 1; int _one() => 1;

thank you, I agree this way is better, please check my changes

How expensive is a lookup? Shouldn't it O(1)? So we are basically avoiding O(2)?

Yes, we are avoiding O(2) and lowering the constant factor. I hope, that assumes the map is actually implemented efficiently, but it's at least possible.

Also, for degenerate cases, hash table lookup is linear. And quick-sort is quadratic. We always hope to not hit those cases, but when we do, it's better to have a low constant.

For library code that other people will use, especially low-level code like this, efficiency is important.
If all your helper libraries are 10% slower than necessary, then your entire application is at least 10% slower than necessary. (If helper libraries use helper libraries, it may compound.)
And there is nothing you can do about it.

So when you provide code for others to depend on, to build their algorithms and applications on, you should never say "a constant factor doesn't matter", because to end users, the difference between 15ms and 17ms may be a missed animation frame, and that matters.

(So, that's my pocket philosophy about writing code for others: The only person who can say whether performance is "good enough" is the application writer. A helper library doesn't know that. It may choose how it optimizes, for speed or size or a tradeoff, but it shouldn't say "it's good enough" and leave easy optimizations on the table. Every level below the application has to do their best and not be part of the problem.)

The one thing we should actually do is to measure. If operator[] and operator[]= are so heavily optimizied that they beat update even if they do the same lookup twice, then we need to optimize update too. I want to choose the operation that has the best possible performance, rather than using workarounds that are faster right now, and never come back and check if they stay that way. If update isn't faster today, we should file an issue, so that it is tomorrow.

(I can see that I flip between pragmatic and ideological reasoning - be fast, because it matters, but don't be fast by doing it the wrong way, even if it is faster today. Tradeoffs. It's tradeoffs all the way down. So reasonable people can disagree on where to trade what. )

I'm not disagreeing, just wanted to understand the reasoning.

Thanks for the explanation.

(I actually didn't even know .update could be potentially faster, I never considered that it would avoid a lookup.)

…com/pvlKryu/collection into fix-issue-355-countFrequency-method

This reverts commit 1fe3d16.

lrhn · 2024-09-29T21:47:40Z

lib/src/iterable_extensions.dart

@@ -601,6 +601,22 @@ extension IterableExtension<T> on Iterable<T> {
      yield slice;
    }
  }
+
+  /// Returns a map where the keys are the unique elements of the iterable


Single line first paragraph. Format as sentences.
Don't use Returns for a getter. (Don't start any DartDoc with it, there is always a better word).

I'd say:

/// The count of occurrences of each element. /// /// The map has an entry for each distinct element of this iterable, /// as determined by `==`, where the value is the number of eelements /// in this iterable which are equal to the key. /// If there are elements that are equal, but not identical, its unspecified /// which of the elements is used as the key. /// /// For example `['a', 'b', 'c', 'b', 'c', 'c'].frequencies` /// is a map with entries like `{'a': , 'b': 2, 'c': 3}`.

lrhn · 2024-09-29T21:49:43Z

test/extensions_test.dart

+        expect(frequencyMap, {42: 1});
+      });
+
+      test('should return correct frequency map for Queue of integers', () {


It's probably overkill to go throguh all kinds of iterables.
I'd rather have more diversity in the elements.

For example having elements that are equal, but not identical, or having elements that are records.
It's the building of the map that is tricky, iterating the iterable will probably work.

added get frequencies tests extended group

lrhn · 2024-09-29T21:51:45Z

@natebosch WDYT. Is this worth adding?
(it's pretty simple, so no big complexity add, and it's something that is often hard for people to write efficiently.)

natebosch · 2024-09-30T18:44:45Z

@natebosch WDYT. Is this worth adding?

Yes, I think it is worth adding. It's not as general as groupFoldBy without the keyOf callback, but you can accomplish the same with a .map call. Getting frequencies by == is common enough to have the specialized implementation.

lrhn · 2024-10-01T10:42:30Z

lib/src/iterable_extensions.dart

+  /// ```
+  ///
+  /// Note: This method uses `==` to compare elements. 
+  /// For collections # `List`, `Set`, or `Map`, deep equality is not checked. 


... where not even shallow equality is checked, two lists are equal only if they are the same instance.

If that is a use-case you want to support, maybe we should to add a parameter for the equality, to allow you to ask for const ListEquality() for the elements:

Map<T, int> frequencies([Equality<T>? equality]) { var frequenceyMap = equality == null ? <T, int>{} : equality is IdentityEquality ? LinkedHashMap<T, int>.identity() : LinkedHashMap<T, int>(equality.equals, equality.hash); // ... }

or have a parallel ... frequenciesBy(Equality<T> equality) ....

Then you can do nested list equality as .frequenciesBy(const ListEquality(ListEquality())).

I think this is "shallow" as it relates to this, not as it relates to the elements of this that are also collection typed.

ACK. In that case the wording is at least ambiguous.
Consider:

/// For elements that are collections, like `List`, `Set` or `Map`, the `==` comparison only /// checks the identity of the collection, not its contents.

lrhn · 2024-10-01T10:42:34Z

lib/src/iterable_extensions.dart

+  /// but not identical (`identical`), 
+  /// it is unspecified which of the elements is used as the key in the map. 
+  /// For example, if there are multiple lists with the same content, 
+  /// the map will only keep one of them as a key.


Not a good example, though, since list equality is identity. Same below ...

lrhn · 2024-10-01T10:51:49Z

test/extensions_test.dart

+
+    group('get frequencies tests extended', () {
+      test('list of equal but not identical strings', () {
+        var list = ['apple', String.fromCharCodes('apple'.codeUnits)];


String is not a good choice for this test because those will be identical when compiled for web.

I'd drop this one, and just use the MyObject test below.
(Or keep this, but document that identity may differ between platforms.)

lrhn · 2024-10-01T10:52:25Z

test/extensions_test.dart

+        expect(frequencyMap, {const MyObject(1): 2, const MyObject(2): 1});
+      });
+
+      test('list with equal numbers but different types', () {


Good test. There is no way around web considering them all identical here, but different classes being equal is an important test for native numbers.

(I was about to suggest testing with double.nan, but ... don't. You can't look it up in the map afterwards, and the default implementation of Map.entries actually looks up the value, so you can't assume to be able to even inspect the returned map.)

lrhn · 2024-10-01T10:59:59Z

test/extensions_test.dart

+      });
+
+      test('list with elements that are objects', () {
+        var list = [const MyObject(1), const MyObject(1), const MyObject(2)];


Don't use const here, because that makes the objects actually identical, so it doesn't check equality being different from identitiy.
Or have one const and one not.
Or add a second field to MyObject, where only one, say the first one is used for equality: [const MyObject(1, "a"), const MyObject(1, "b") ...
would be equal but not identical.

lrhn · 2024-10-01T11:08:38Z

test/extensions_test.dart

+      test('should handle empty List', () {
+        var list = [];
+        var frequencyMap = list.frequencies;
+        expect(frequencyMap, {});


Consider isEmpty as matcher. I think it works for maps too, and it should a more precise an error message like "is not empty".

lrhn · 2024-10-01T11:11:50Z

test/extensions_test.dart

@@ -1352,6 +1353,122 @@ void main() {
        expect(l3.toList(), [4, 5]);
      });
    });
+    group('get frequencies tests', () {


Just 'frequenices', it's cleaner.

The names of groups and tests are concatenated with a space between them, so they can rely on context. Things like "should be correct" is a given for tests, so it doesn't add value. Consider:

group('frequencies', () { test('of integers', () { ... }); test('of strings', () { ... }); test('of empty iterable', () { ... }); test('of empty single element iterable', () { ... }); group('(when equality is not identity)', () { test('of records', () { ... }); /// ... }); });

These names are only important when a test fails.
At that point, they should give enough context to make the error message meaningful.

Take "frequencies of single element iterable" with an error (if the result was empty) of

Expected: {42: 1} Actual: {} Which: has different length and is missing map key <42>

With that headline, it's fairly clear where and what the problem is.

lrhn · 2024-10-01T11:15:40Z

test/extensions_test.dart

+        expect(frequencyMap, {'a': 1, 'b': 1, 'c': 1});
+      });
+
+      test('should handle empty Set', () {


I would drop the tests for sets and queues unless there is a reason to believe that the implementation treats them differently (which it doesn't).

We're not testing iteration, that is assumed to work, so all that matters is the sequence of values we get by iterating, and a List is fine for that.

(If we start trying to detect lists or sets, to do more efficient iteration or maybe assuming that sets cannot have duplicates so we don't need to count - which isn't true because sets can have non-standard equality - then there is reason to test with, and without, those types specifically.)

lrhn · 2024-10-01T11:17:11Z

test/extensions_test.dart

+        expect(frequencyMap, {'apple': 2});
+      });
+
+      test('list of records', () {


(So for naming, I'd drop the "list of" here, what is important is that it's records being compared, and it being a list isn't important to the test.
Maybe just test('of records', () { ... }); with the changes above.)

lrhn

Looking good, but if doing shallow comparison of lists is a goal, we need to parameterize with an equality.

pvlKryu · 2024-10-01T11:38:38Z

@lrhn Thank you very much for the quick feedback and for the detailed and helpful comments. I need some time to review everything you mentioned and correct it. I will let you know soon!

mosuem · 2024-10-21T07:34:49Z

Closing as the dart-lang/collection repository is merged into the dart-lang/core monorepo. Please re-open this PR there!

pvlKryu added 2 commits August 2, 2024 20:08

issue-335 countFrequency method added

ddcad5b

issue-335 countFrequency tests added

c2a7133

pvlKryu mentioned this pull request Aug 2, 2024

Feature Request: Add countFrequency method to Iterable in the collection package dart-lang/core#679

Open

issue-335 countFrequency comments edited

0c833d8

mateusfccp reviewed Sep 2, 2024

View reviewed changes

lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved

Update lib/src/iterable_extensions.dart

b444e37

Co-authored-by: Mateus Felipe C. C. Pinto <[email protected]>

lrhn reviewed Sep 13, 2024

View reviewed changes

pvlKryu added 6 commits September 29, 2024 13:30

issue-335 converted countFrequency to getter

1fe3d16

Merge branch 'fix-issue-355-countFrequency-method' of https://github.…

c5e18a2

…com/pvlKryu/collection into fix-issue-355-countFrequency-method

issue-335 avoided two lookups per item

8acd858

Revert "issue-335 converted countFrequency to getter"

facd926

This reverts commit 1fe3d16.

issue-335 converted countFrequency to getter

f4bbe43

issue-335 returned the desired string length back

9c4cf47

pvlKryu changed the title ~~Fix issue 355 count frequency method to iterable~~ Fix issue 355 added get frequency getter to iterable Sep 29, 2024

lrhn reviewed Sep 29, 2024

View reviewed changes

lrhn requested a review from natebosch September 29, 2024 21:50

pvlKryu added 2 commits October 1, 2024 10:44

issue-335 corrected the description in the comment

da963a5

issue-335 added 'get frequencies tests extended' group

2373566

lrhn reviewed Oct 1, 2024

View reviewed changes

mosuem closed this Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 355 added get frequency getter to iterable #357

Fix issue 355 added get frequency getter to iterable #357

pvlKryu commented Aug 2, 2024 •

edited

Loading

pvlKryu commented Sep 3, 2024

pvlKryu commented Sep 3, 2024

mateusfccp commented Sep 3, 2024

lrhn Sep 13, 2024

pvlKryu Sep 29, 2024

lrhn Sep 13, 2024

pvlKryu Sep 29, 2024

mateusfccp Oct 1, 2024

lrhn Oct 1, 2024

mateusfccp Oct 1, 2024 •

edited

Loading

lrhn Sep 29, 2024

pvlKryu Oct 1, 2024

lrhn Sep 29, 2024

pvlKryu Oct 1, 2024

lrhn commented Sep 29, 2024

natebosch commented Sep 30, 2024

lrhn Oct 1, 2024 •

edited

Loading

natebosch Oct 2, 2024

lrhn Oct 3, 2024

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024

lrhn Oct 1, 2024

lrhn left a comment

pvlKryu commented Oct 1, 2024

mosuem commented Oct 21, 2024

Fix issue 355 added get frequency getter to iterable #357

Fix issue 355 added get frequency getter to iterable #357

Conversation

pvlKryu commented Aug 2, 2024 • edited Loading

pvlKryu commented Sep 3, 2024

pvlKryu commented Sep 3, 2024

mateusfccp commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateusfccp Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn commented Sep 29, 2024

natebosch commented Sep 30, 2024

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

lrhn Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn left a comment

Choose a reason for hiding this comment

pvlKryu commented Oct 1, 2024

mosuem commented Oct 21, 2024

pvlKryu commented Aug 2, 2024 •

edited

Loading

mateusfccp Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading

lrhn Oct 1, 2024 •

edited

Loading