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

Precompilation warnings? #296

Closed
navidcy opened this issue Apr 8, 2023 · 16 comments · Fixed by #369
Closed

Precompilation warnings? #296

navidcy opened this issue Apr 8, 2023 · 16 comments · Fixed by #369
Labels
bug 🐞 Something isn't working dependency 🦮 Dependencies on other packages
Milestone

Comments

@navidcy
Copy link
Collaborator

navidcy commented Apr 8, 2023

Why all these warnings? Also where are they coming from? The warning mentions, e.g., in module SpeedyWeather at util.jl:504 but where is this util.jl file?

[ Info: Precompiling SpeedyWeather [9e226e20-d153-4fed-8a5b-493def4f21a9]
WARNING: Method definition (::Type{SpeedyWeather.Parameters{Model} where Model<:SpeedyWeather.ModelSetup})() in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/default_parameters.jl:345.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition Type##kw(Any, Type{SpeedyWeather.Parameters{Model} where Model<:SpeedyWeather.ModelSetup}) in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/default_parameters.jl:345.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition (::Type{SpeedyWeather.ColumnVariables{NF} where NF<:AbstractFloat})() in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/define_column.jl:111.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition Type##kw(Any, Type{SpeedyWeather.ColumnVariables{NF} where NF<:AbstractFloat}) in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/define_column.jl:111.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition (::Type{SpeedyWeather.LinearDrag{NF} where NF<:Real})() in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/parameter_structs.jl:72.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition Type##kw(Any, Type{SpeedyWeather.LinearDrag{NF} where NF<:Real}) in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/parameter_structs.jl:72.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition (::Type{SpeedyWeather.HeldSuarez{NF} where NF<:Real})() in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/parameter_structs.jl:90.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition Type##kw(Any, Type{SpeedyWeather.HeldSuarez{NF} where NF<:Real}) in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/physics/parameter_structs.jl:90.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition (::Type{SpeedyWeather.Output{NF} where NF<:Union{Float32, Float64}})() in module SpeedyWeather at util.jl:504 overwritten at /Users/navid/Research/SpeedyWeather.jl/src/output/output.jl:43.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition speedstring(Any) in module ProgressMeter at /Users/navid/.julia/packages/ProgressMeter/sN2xr/src/ProgressMeter.jl:683 overwritten in module SpeedyWeather at /Users/navid/Research/SpeedyWeather.jl/src/output/pretty_printing.jl:29.
  ** incremental compilation may be fatally broken for this module **
@milankl
Copy link
Member

milankl commented Apr 8, 2023 via email

@milankl
Copy link
Member

milankl commented Apr 8, 2023 via email

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 8, 2023

Haha... I figured it out and deleted my question. But your answer came anyway! :)

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 8, 2023

Hm... This might be "type piracy"?
Yes, I didn’t want to go down the route of implementing the speed estimate properly in ProgressMeter.jl and instead went for the easy way to just locally redefine their function so that it takes an additional argument so that the default time/iteration can be translated to simulated time/time. I’ve not have problems with this before though!

Do we need to explicitly import it perhaps?

import ProgressMeter: speedstring

before adding the new method?

I don't think it's type piracy because you are defining a method for a type that SpeedyWeather defines, so that's OK!

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 8, 2023

I think the problem is that we define

speedstring(sec_per_iter, dt_in_sec=SpeedyWeather.DT_IN_SEC)

which, because we provide a default for dt_in_sec=SpeedyWeather.DT_IN_SEC it implies a re-definition of the method speedstring(::Any). That's why it's complaining...

@milankl
Copy link
Member

milankl commented Apr 10, 2023

I remember having had precompilation warnings for that a long time ago, but not recently. So I don't know why this is coming up now for you.

@milankl milankl added bug 🐞 Something isn't working dependency 🦮 Dependencies on other packages labels Apr 10, 2023
@milankl milankl added this to the v0.5 milestone Apr 10, 2023
@milankl
Copy link
Member

milankl commented May 11, 2023

Just ran into this problem again after adding SpeedyWeather.jl the first time with v1.9 It looks like the problem arises from constructors like

Base.@kwdef AB{NF<:Real}
     a::NF = 1.0
     b::NF = 2.0
end

AB(;kwargs...) = AB{Float64}(;kwargs...)

While the 2nd method overwrites AB() it also allows AB(b=2), which why I defined it initially. I saw on Julia slack that @charleskawczynski ran into a similar problem. Maybe you have some ideas?

@navidcy
Copy link
Collaborator Author

navidcy commented May 11, 2023

What is AB(;kwargs...) = AB{Float64}(;kwargs...) for?

@milankl
Copy link
Member

milankl commented May 11, 2023

Essentially so that one can do AB(b=2) (which isn't defined, because b::Int, but a::Float64 in that case) and it would convert b=2 to b=2.0

It means that something like run_speedy(diffusion=HyperDiffusion(time_scale=3) would throw an error, which is counterintuitive and unnecessary?

@navidcy
Copy link
Collaborator Author

navidcy commented May 11, 2023

Btw, the answer to my question was already in your previous post but the coffee hadn't gone to my brain....

@milankl
Copy link
Member

milankl commented May 11, 2023

It would be possible to do

Base.@kwdef struct ABC{NF<:Real}
      a::NF = convert(NF,1)
      b::NF = convert(NF,2)
end

ABC(;kwargs...) = ABC{Float64}(;kwargs...)

because it doesn't define ABC() and so the ABC(;kwargs...) can be added later

@charleskawczynski
Copy link

Yeah, Base.@kwdef doesn't always provide the constructor that you may want for users-- it's easy if all the types are concrete and separate. Otherwise, I tend to just write my own inner constructor and do diagonalization there. Maybe there's opportunity for a new macro?

@milankl
Copy link
Member

milankl commented May 11, 2023

Can you point me to an example? Cause what I often need is

  • a struct ABC{T} where all/most fields are of type T, so that I can dispatch over it
  • default values for the fields directly written into the struct definition
  • and a generous constructor that takes any keyword arguments, promotes their type somehow to T and creates a struct{T} with the values of the arguments

@charleskawczynski
Copy link

charleskawczynski commented May 11, 2023

It sounds like what you're looking for is:

struct ABC{NF<:Real}
      a::NF
      b::NF
      function ABC{NF}(; a = NF(1), b = NF(2)) where {NF}
            return new{NF}(a, b)
      end
      ABC(; kwargs...) = ABC{promote_type(typeof.(values(kwargs))...)}(;kwargs)
      ABC() = ABC{Float64}()
end

?
It's maybe not the prettiest way to write it, but I think this will do what you want for at least for all float type structs. And of course you can move the inner constructors to outer constructors if you need access to the default constructor

@milankl
Copy link
Member

milankl commented May 22, 2023

With the restructuring of #327 these warnings should disappear, moving this therefore to v0.6

@milankl milankl modified the milestones: v0.5, v0.6 May 22, 2023
@milankl
Copy link
Member

milankl commented Jun 15, 2023

In main we're now down to one single precompilation warning

┌ SpeedyWeather [9e226e20-d153-4fed-8a5b-493def4f21a9]
│  WARNING: Method definition speedstring(Any) in module ProgressMeter at /Users/milan/.julia/packages/ProgressMeter/sN2xr/src/ProgressMeter.jl:683 overwritten in module SpeedyWeather at /Users/milan/.julia/packages/SpeedyWeather/oRCBd/src/output/feedback.jl:201.** incremental compilation may be fatally broken for this module **

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working dependency 🦮 Dependencies on other packages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants