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

Type inference problem in color conversions #4020

Closed
timholy opened this issue Aug 11, 2013 · 5 comments
Closed

Type inference problem in color conversions #4020

timholy opened this issue Aug 11, 2013 · 5 comments

Comments

@timholy
Copy link
Member

timholy commented Aug 11, 2013

The following code allocates memory when it seems like it shouldn't:

using Color

import Base.uint32

function uint32(c::ColorValue, n)
    s = uint32(0)
    for i = 1:n
        cr = 255.0*c.r
        ci = iround(Uint32, cr)
        cis = ci << 16
        s += ci
    end
    s
end

c = RGB(rand(),rand(),rand())
uint32(c, 10^7)
@timholy
Copy link
Member Author

timholy commented Aug 11, 2013

@JeffBezanson, if you'd like I can start a test/inference.jl file and start putting examples such as these inside a giant@allocated check.

@JeffBezanson
Copy link
Member

Actually the problem is that Uint32+Uint32 gives a Uint64. I do sometimes
wonder about that decision. If you want s to be a Uint32, the best approach
is "s::Uint32 = 0". Then you would only need to write the type once. For
some reason this syntax has not been that popular.
On Aug 11, 2013 6:52 PM, "Tim Holy" notifications@github.com wrote:

@JeffBezanson https://github.com/JeffBezanson, if you'd like I can
start a test/inference.jl file and start putting examples such as these
inside a giant@allocated check.


Reply to this email directly or view it on GitHubhttps://github.com//issues/4020#issuecomment-22467148
.

@timholy
Copy link
Member Author

timholy commented Aug 12, 2013

Indeed that fixes it, thanks!

Actually the problem is that Uint32+Uint32 gives a Uint64. I do sometimes
wonder about that decision.

I do too, especially since Float32+Float32 = Float32.

the best approach is "s::Uint32 = 0" ... For some reason this syntax has not been that popular.

It's quite popular with me :-). I had in fact put such declarations on cr and ci (deleted while writing the issue text), but had not thought to do s. When I'm solving such problems I don't have a systematic way of figuring out which variable is causing the problem, so in general I sprinkle them everywhere I notice (but I don't always quickly notice the important spot, like here). Any tips? Do you just work your way through the disassembly? I am still not sufficiently fluent to make that easy on bigger functions, although I probably should have done that for this one.

What are the odds of writing a tool to help identify such problems? For example,

julia> boxedvars(uint32, (ColorValue, Int))
/tmp/uint32test.jl, line 11: s

Over the past few days in my own coding I've probably spent ~15% of my time chasing down such issues, so I'm not entirely causal in asking about this. (Oh no, I can see another Profile-scale job coming...)

@JeffBezanson
Copy link
Member

I believe @astrieanna has been working on a tool for identifying things like this.

@timholy
Copy link
Member Author

timholy commented Aug 13, 2013

Awesome!

IanButterworth pushed a commit that referenced this issue Oct 7, 2024
…0d22b (#56032)

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: release-1.11
Julia branch: backports-release-1.11
Old commit: 6ceafca8e
New commit: aba90d22b
Julia version: 1.11.0
Pkg version: 1.11.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@6ceafca...aba90d2

```
$ git log --oneline 6ceafca8e..aba90d22b
aba90d22b Merge pull request #4037 from JuliaLang/backports-release-1.11
76eaa4caa Fix julia#55850 by using safe_realpath instead of abspath in projname (#4025)
df38587fb warn if General is installed via the old slow methods (#4022)
1475b628a update package extension naming docs (#4000)
72dc85e80 Tweak sentence syntax in getting-started.md (#4020)
0b2397089 make `add` and `dev` on a package remove it from the set of weak dependencies (#3865)
ee2d51054 collect e.g. weak deps from project even if it is not a package (#3852)
```

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
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

No branches or pull requests

2 participants