Skip to content

Conversation

QWYNG
Copy link
Contributor

@QWYNG QWYNG commented Oct 18, 2019

Hi! Thank you for great programming language.
add frequencies and frequencies_by.

This is a discussion about this method.
https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/elixir-lang-core/hBRKIIy8QKE/2JlhpblqBgAJ

If any points to fix, don't hesitate to say.

@Eiji7
Copy link
Contributor

Eiji7 commented Oct 18, 2019

@QWYNG How about using reduce and implement only frequencies_by/1 and frequencies_by/2 (see Enum.group_by/2 and Enum.group_by/3)?

Here is my proposition:

defmodule Example do
  @spec frequencies_by(Enum.t(), (Enum.element() -> any)) :: map
  def frequencies_by(enumerable, key_fun \\ fn x -> x end) when is_function(key_fun) do
    Enum.reduce(enumerable, %{}, fn entry, acc ->
      key = key_fun.(entry)
      Map.update(acc, key, 1, &(&1 + 1))
    end)
  end
end

@josevalim
Copy link
Member

Yes, we should go with @Eiji7 proposal, as it is quite more efficient. :)

@QWYNG
Copy link
Contributor Author

QWYNG commented Oct 18, 2019

@Eiji7 @josevalim
Thank you for the proposal!
I'll fix.

@QWYNG QWYNG closed this Oct 18, 2019
@QWYNG QWYNG reopened this Oct 18, 2019
@QWYNG
Copy link
Contributor Author

QWYNG commented Oct 18, 2019

sorry, I accidentally closed PR once

@QWYNG QWYNG changed the title add Enum.frequencies and Enum.frequencies_by add Enum.frequencies_by Oct 18, 2019
@josevalim
Copy link
Member

Sorry @QWYNG. I think there was some confusion. We should have two functions: frequencies/1 and frequencies_by/2. Their implementation is very close to each other but given no _by functions today have a default argument, I don't think we should introduce one here. Can you please amend accordingly? Sorry for the confusion and thanks!

@QWYNG
Copy link
Contributor Author

QWYNG commented Oct 18, 2019

@josevalim
OK! That means we need two functions like min and min_by.
That way I think frequencies_by/2 don't need a default argument.
I'll amend.

@QWYNG QWYNG changed the title add Enum.frequencies_by add Enum.frequencies and Enum.frequencies_by Oct 19, 2019
@whatyouhide whatyouhide merged commit b7b6480 into elixir-lang:master Oct 19, 2019
@whatyouhide
Copy link
Member

Thank you @QWYNG! 💟

@QWYNG
Copy link
Contributor Author

QWYNG commented Oct 19, 2019

@whatyouhide
You are Welcome!
@Eiji7 @josevalim @whatyouhide
Thank you all! Elixir is a great language!

@QWYNG QWYNG deleted the enum-frequencies branch October 19, 2019 11:51
def frequencies_by(enumerable, key_fun) when is_function(key_fun) do
reduce(enumerable, %{}, fn entry, acc ->
key = key_fun.(entry)
Map.update(acc, key, 1, &(&1 + 1))
Copy link
Member

@michalmuskala michalmuskala Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's a library function and it should be as efficient as possible, could we manually expand the call to Map.update to avoid the fun overhead? It's tiny, but things like that add up, especially in library functions.

@josevalim
Copy link
Member

josevalim commented Oct 20, 2019 via email

@QWYNG
Copy link
Contributor Author

QWYNG commented Oct 23, 2019

@michalmuskala @josevalim
Like this?

  def frequencies(enumerable) do
    reduce(enumerable, %{}, fn key, acc ->
      case acc do
        %{^key => value} ->
          Map.put(acc, key, value + 1)
    
        %{} ->
          Map.put(acc, key, 1)
      end
    end)
  end

If it's alright with @michalmuskala, I will create PR. :)

@josevalim
Copy link
Member

I would even do this:

  def frequencies(enumerable) do
    reduce(enumerable, %{}, fn key, acc ->
      case acc do
        %{^key => value} -> %{acc | key => value + 1}
        %{} -> Map.put(acc, key, 1)
      end
    end)
  end

A PR is welcome!

@michalmuskala
Copy link
Member

Please go ahead @QWYNG. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants