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

replace itrunc(Uint8, 1234) => mod(1234, Uint8), etc. [fix #8646] #8648

Merged
merged 4 commits into from
Oct 17, 2014

Conversation

StefanKarpinski
Copy link
Member

Here's the proposal I put forth in #8646 to replace itrunc(Uint8, 1234) etc. with mod(1234, Uint8). Not necessarily how we want to do this, but at least it (hopefully) identifies all the places in base where this needs to be changed.

@@ -174,8 +174,8 @@ for to in tuple(IntTypes...,Char), from in tuple(IntTypes...,Char,Bool)
end
Copy link
Member

Choose a reason for hiding this comment

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

you missed an itrunc on line 172

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, yes. it was initially unclear to me if that was this kind of itrunc, but it must be.

@vtjnash
Copy link
Member

vtjnash commented Oct 14, 2014

it seems more idiomatic to do mod(Uint8, 1234), but I'm not sure if this is punning too much on mod. In some respects we just want a syntactic way to specify output types (I think I saw mod{Uint8}(1234) proposed recently. I've suggested mod(1234) |> Uint8 previously. Back when Jeff made the PR adding checked conversions, I took a stab at implementing a more general coerce method: #8420 (comment) that is perhaps similar to this

@StefanKarpinski
Copy link
Member Author

The reverse argument order of mod(T, x) is pretty awkward and annoying, but I can't get over the fact that this is very clearly a modulus operation. I'm not fond of the mod{Uint8}(1234) syntax either. One thing that might be more palatable is adding these methods to rem and writing 1234 % Uint8. Since the sign of the result is the difference between rem and mod but there's only one possible modular result when modding into an integer type, this seems just as coherent as using mod.

@JeffBezanson
Copy link
Member

I liked the idea of using trunc and having it truncate bits at both ends. That also has the benefit of removing the extra itrunc etc. names, and adding defined behavior to more conversions.

@StefanKarpinski
Copy link
Member Author

I think @simonbyrne made some pretty compelling arguments against cramming all of that into trunc.

@JeffBezanson
Copy link
Member

Ok, I guess I won't argue with Simon and Steve J on that. We can use mod or % here, and also replace itrunc et. al. with trunc et. al. and make it throw appropriate InexactErrors.

@StefanKarpinski
Copy link
Member Author

I kind of like 1234 % Uint8 the more I think about it.

@quinnj
Copy link
Member

quinnj commented Oct 15, 2014

The only problem with using % is it's very non-obvious. No one is ever going to just try 1234 % Uint8. That said, I think it is a great way to express this and once you see/learn it, it does have a niceness to it, so I think that's a good reason to go with it.

@StefanKarpinski
Copy link
Member Author

We could make convert(Uint8, 1234) print a helpful message to try 1234 % Uint8.

@JeffBezanson
Copy link
Member

Just subjective, but mod somehow feels more appropriate to me. Not a strong opinion though.

@StefanKarpinski
Copy link
Member Author

The operators syntax really feels right though and it's really concise :-)

@JeffBezanson
Copy link
Member

You humans and your infix.

@StefanKarpinski
Copy link
Member Author

Humans are the worst, JeffBot2000.

@StefanKarpinski
Copy link
Member Author

@simonbyrne, @stevengj – what do you think of the latest incarnation of this using %?

@vtjnash
Copy link
Member

vtjnash commented Oct 16, 2014

i'm still waiting for convert to get it's own (strong bad) operator #1470 (comment)

@simonbyrne
Copy link
Contributor

% seems fine to me. What's the plan for the current "unsafe" itrunc(::FloatingPoint) function?

@StefanKarpinski
Copy link
Member Author

Can that just be called unsafe_trunc and safe itrunc gets folded into trunc?

@StefanKarpinski
Copy link
Member Author

@JeffBezanson, are you cool with me merging this? This is going to cause a lot of updates to packages that just had to change convert to itrunc, but it seems unavoidable.

@JeffBezanson
Copy link
Member

Yes, I'm ok with it. More specifically, I don't have a better alternative to offer. It's kind of unfortunate that integer truncation has an operator, and convert doesn't; I'll second @vtjnash there.

@StefanKarpinski StefanKarpinski merged commit 51b6e03 into master Oct 17, 2014
@tkelman
Copy link
Contributor

tkelman commented Oct 17, 2014

This would seem less odd if there were more other cases of value operator type. But it works and separates concerns, so I'll get over it.

@StefanKarpinski
Copy link
Member Author

That is true. We could, of course, make more instances – e.g. x @ Int for conversion?

@toivoh
Copy link
Contributor

toivoh commented Oct 17, 2014

I agree that it could be nice to have syntax for conversion too, though that particular syntax seems dangerously close to macro invocation?

@quinnj
Copy link
Member

quinnj commented Oct 17, 2014

I'm still a fan of x as Int.

@joehuchette
Copy link
Member

With an infix operator for convert, would it be possible to allow their use in method definition? E.g. have

foo(x, y as Int, z) = x + y + z

equivalent to

foo(x, y, z) = foo(x, convert(Int, y), z)
foo(x, y::Int, z) = x + y + z

@JeffBezanson
Copy link
Member

@joehuchette That is a really interesting idea!

@joehuchette
Copy link
Member

It would be great for wrapping some of the C optimization libraries we work with (e.g. CPLEX, SCIP), where this is a common idiom.

@StefanKarpinski
Copy link
Member Author

Yes, that's a really good idea. Syntactic interplay with type annotation on the same argument is a bit messy though.

@stevengj
Copy link
Member

I like that idea too. (I think you would just disallow type annotation on the same argument: the as Foo would be the type annotation, because it would just be syntactic sugar as @joehuchette proposed).

@StefanKarpinski
Copy link
Member Author

There are inevitably going to be cases where you want both dispatch constraints and conversion. But maybe that doesn't matter.

@vtjnash
Copy link
Member

vtjnash commented Oct 17, 2014

F(x::Real as Int)
F(x::Real... as (Int...))
F(x as Int = 23 as Float32)

That last one is a bit strange, but they look quite readable

+1

@stevengj
Copy link
Member

Can we move this thread to 8710?

@JeffBezanson JeffBezanson deleted the sk/modpocalypse branch October 25, 2014 17:27
@damiendr
Copy link
Contributor

damiendr commented Nov 6, 2015

As far as I can tell there is no mention of this in the documentation.

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.

10 participants