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

Add and/or operations for series #57

Merged
merged 7 commits into from
Sep 10, 2021

Conversation

drowzy
Copy link
Contributor

@drowzy drowzy commented Sep 8, 2021

Adds Explorer.Series.and_/2 & Explorer.Series.or_/2 by using bitor/and on the series.

closes #56

@josevalim
Copy link
Member

Btw, Elixir will be fine with invoking Explorer.Series.and(foo, bar). So we can keep the name as and/or, but you will need to declare it as def left and right do...end. :)

@drowzy
Copy link
Contributor Author

drowzy commented Sep 8, 2021

Btw, Elixir will be fine with invoking Explorer.Series.and(foo, bar). So we can keep the name as and/or, but you will need to declare it as def left and right do...end. :)

Hah! Did not know that! Thank you :)!

Though I'm running into to trouble on Kernel being imported and length being excluded to avoid the conflict with the locally defined length/1. However now I'm getting a conflict on and instead.
Since and is used in guards so excluding it fails to compile. Any suggestions how to resolve that?

@cigrainger
Copy link
Member

Thanks for this! Re: the kernel conflicts, one way I would approach it (and I'm not sure if it's the right way!) is to pattern match and then defdelegate after the exhausted pattern matches. So with length as an example:

  @spec length(series :: Series.t()) :: integer()
  def length(%Series{} = series), do: apply_impl(series, :length)
  defdelegate length(other), to: Kernel

@josevalim
Copy link
Member

@cigrainger unfortunately that won't work for and/or if used in guards because guards require specific constructs.

@drowzy I would try this at the top of the file:

import Kernel, except: [and: 2, or: 2]
alias Kernel, as: K

Now every time there is a and/or from Kernel, you rewrite left and right to K.and(left, right). It is up to @cigrainger though to say if he wants to leave with this.

However, I would use def left and right do only on the API definition. In the backend, I would call it binary_and and binary_or, so we don't have to handle the ambiguity in the backend itself (only on the API facade). WDYT?

@drowzy
Copy link
Contributor Author

drowzy commented Sep 9, 2021

@josevalim Thanks for the advice :)! The API definition is update to use and/or and the behaviour and backend uses binary_and/binary_or.

Copy link
Member

@cigrainger cigrainger left a comment

Choose a reason for hiding this comment

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

Just one thing to fix. The readability of relatively complex guards is definitely taking a hit, but I think it's worth it for the functionality.

lib/explorer/series.ex Outdated Show resolved Hide resolved
drowzy and others added 2 commits September 10, 2021 08:50
Co-authored-by: Christopher Grainger <cigrainger@users.noreply.github.com>
@cigrainger cigrainger merged commit c5e55c7 into elixir-explorer:main Sep 10, 2021
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.

Clarification / Question: Dataframe.filter AND/OR mask?
3 participants