- 
                Notifications
    You must be signed in to change notification settings 
- Fork 250
          Fix type instability in AbstractOperations
          #4870
        
          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
base: main
Are you sure you want to change the base?
  
    Fix type instability in AbstractOperations
  
  #4870
              Conversation
…om:CliMA/Oceananigans.jl into ss/fix-abstract-operation-type-instability
| mask = zero(eltype(operand))) | ||
| mask = zero(eltype(operand))) where {LX, LY, LZ} | ||
| condition = validate_condition(condition, operand) | ||
| LX, LY, LZ = location(operand) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this has an effect (but tell me if its not true). I think this compiles identically. We use the function since its the recommendation of YASGuide (eg type parameters should be used for dispatch, not to access type info). It's not that important, but wanted to mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a matter of passing arguments directly as parameteric types, which is type unstable, rather than inferring a type. The constructor cannot infer the values of an argument, but can infer the type of the arguments that are passed. Check the difference between these two constructors (which basically build the same object with just a different name)
struct InstableType{T1, T2, T3} end
struct StableType{T1, T2, T3} end
StableType(t::Tuple{T1, T2, T3}) where {T1, T2, T3} = StableType{T1, T2, T3}()
InstableType(T::Tuple) = InstableType{T[1], T[2], T[3]}()
@code_warntype StableType((1.0, 1.f0, Float16(1.0))) # This is type stable
@code_warntype InstableType((Float64, Float32, Float16)) # This is type - unstableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you are right. In the above case, there is no issue because LX, LY, LZ should be inferred in the function call. I will revert it. The problem arises only for constructors like this
Oceananigans.jl/src/AbstractOperations/binary_operations.jl
Lines 38 to 43 in f07428b
| function _binary_operation(Lc, op, a, b, La, Lb, grid) | |
| ▶a = interpolation_operator(La, Lc) | |
| ▶b = interpolation_operator(Lb, Lc) | |
| return BinaryOperation{Lc[1], Lc[2], Lc[3]}(op, a, b, ▶a, ▶b, grid) | |
| end | 
which should actually be
 function _binary_operation(Lc::Tuple{LX, LY, LZ}, op, a, b, La, Lb, grid) where {LX, LY, LZ}
      ▶a = interpolation_operator(La, Lc) 
      ▶b = interpolation_operator(Lb, Lc) 
  
     return BinaryOperation{LX, LY, LZ}(op, a, b, ▶a, ▶b, grid) 
 end There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
| After fixing this type - instability, I have noticed I introduced another type - instability in the Field constructor in #4706, which basically just moved the already existing type instability out of the fill halo functions and inside the constructor. | 
closes #4869