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

Implemented bitwise nand and nor; tests and docs included. #40339

Merged
merged 6 commits into from
Apr 9, 2021

Conversation

arvganesh
Copy link
Contributor

@arvganesh arvganesh commented Apr 4, 2021

Addresses #40272

Features included:

  • Implemented bitwise nand and nor (for single, dual, multiple arguments) similar to the implementation of bitwise xor:
    • is an alias for nand
    • is an alias for nor
  • Written tests in all files containing a test for xor (checked many of these by computing the 256-bit binary representation of a number and wrote a script to compute nand or nor digit by digit). Checked using make test-all.
  • Created documentation. I ran make docs to confirm that it worked.
  • Added tab completion for \nand and \nor; \barwedge and \veebar are still functional.

base/bool.jl Outdated
1
```
"""
nand(x::Bool, y::Bool) = ~(x & y)
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried this with just defining a single method as proposed in #40272? I really don't think you need to define 16 methods here.

Copy link
Contributor Author

@arvganesh arvganesh Apr 4, 2021

Choose a reason for hiding this comment

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

I tried it with just the single method but ran into issues when testing nand(missing) and nand(x) where x is an integer (trying the single-arg versions caused an error). Instead of defining 16 methods, I've defined 4:

julia> methods(nand)

# 4 methods for generic function "nand":
[1] nand(::Missing) in Base at missing.jl:101
[2] nand(a::BigInt, b::BigInt) in Base.GMP at gmp.jl:551
[3] nand(x::Integer) in Base at operators.jl:579
[4] nand(x, y) in Base at bool.jl:105

This passes all the tests I've made.

Copy link
Member

Choose a reason for hiding this comment

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

I would skip the one-arg ones as well and just have the two-arg version. nand(x) = x seems questionable to me anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand how nand(x) = x seems questionable, but out of curiosity, what's your thinking on nand(missing)? In my limited knowledge, it seems like we should include it since the majority of other operators have the same property. If not, is there a concept I'm missing / misunderstanding?

Copy link
Member

Choose a reason for hiding this comment

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

For any arbitrary set of values, we can decompose these two functions essentially just directly into their constituent parts:

nand(x...) = ~(&)(x...)
nor(x...) = ~|(x...)

Copy link
Member

Choose a reason for hiding this comment

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

My initial thought was to edge on the more conservative side here and just implement the most minimal version for now. This implementation now seems like a sensible extension to me though, so we might as well go with that.

Copy link
Contributor Author

@arvganesh arvganesh Apr 8, 2021

Choose a reason for hiding this comment

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

Got it, I appreciate your help!

base/gmp.jl Outdated
@@ -547,6 +547,18 @@ end
(-)(x::BigInt) = MPZ.neg(x)
(~)(x::BigInt) = MPZ.com(x)

# nand and nor
nand(a::BigInt, b::BigInt) = MPZ.com(MPZ.and(a, b))
nand(a::BigInt, b::BigInt, c::BigInt) = nand(nand(a, b), c)
Copy link
Member

Choose a reason for hiding this comment

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

This is not what I would expect multi-arg nand to do. In all contexts I have encountered it, it was defined as ~(a & b & c). I think it would be better to just define the two-arg version for now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I can remove the multi-arg versions for now.

Comment on lines 408 to 409
nand(x::T, y::T) where {T<:Integer} = no_op_err("nand", T)
nor(x::T, y::T) where {T<:Integer} = no_op_err("nor", T)
Copy link
Member

Choose a reason for hiding this comment

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

This should not be an error. The idiomatic Julia solution is to use duck-typing, so that nand/nor just work if a type defines ~ and &/|.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll remove these two lines.

@arvganesh arvganesh requested a review from simeonschaub April 5, 2021 05:48
base/gmp.jl Outdated
@@ -547,6 +547,10 @@ end
(-)(x::BigInt) = MPZ.neg(x)
(~)(x::BigInt) = MPZ.com(x)

# nand and nor
nand(a::BigInt, b::BigInt) = MPZ.com(MPZ.and(a, b))
nor(x::BigInt, y::BigInt) = MPZ.com(MPZ.ior(x, y))
Copy link
Member

Choose a reason for hiding this comment

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

Why are these definitions needed? Aren't they equivalent to the generic ~(x & y) and ~(x | y) fallback definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, they are equivalent. I will remove them.

@stevengj
Copy link
Member

stevengj commented Apr 7, 2021

Seems like there should be some specialized methods in base/bitarray.jl similar to those for & and |?

@stevengj stevengj added the maths Mathematical functions label Apr 7, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 8, 2021
@simeonschaub simeonschaub merged commit b838cdf into JuliaLang:master Apr 9, 2021
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants