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

[Dev] Avoid generic types in struct #287

Closed
ocots opened this issue Sep 5, 2024 · 6 comments · Fixed by #294
Closed

[Dev] Avoid generic types in struct #287

ocots opened this issue Sep 5, 2024 · 6 comments · Fixed by #294
Assignees
Labels
internal dev Modification to the code is requested and should not affect user experiment

Comments

@ocots
Copy link
Member

ocots commented Sep 5, 2024

See #271 (comment).

This

struct CallableType{U}
    f::Function
end

should be replaced by

struct CallableType{T<:Function, U}
    f::T
end
@ocots ocots added the internal dev Modification to the code is requested and should not affect user experiment label Sep 5, 2024
@ocots ocots self-assigned this Sep 5, 2024
@amontoison
Copy link
Contributor

Abstract vs concrete type:
https://gensoft.pasteur.fr/docs/julia/1.4.1/manual/performance-tips.html#Avoid-fields-with-abstract-type-1

@ocots
Copy link
Member Author

ocots commented Sep 9, 2024

julia> mutable struct MyType{T<:AbstractFloat}
           a::T
       end

is better choice than

julia> mutable struct MyStillAmbiguousType
           a::AbstractFloat
       end

@ocots
Copy link
Member Author

ocots commented Sep 9, 2024

The same best practices also work for container types:

julia> struct MySimpleContainer{A<:AbstractVector}
           a::A
       end

julia> struct MyAmbiguousContainer{T}
           a::AbstractVector{T}
       end

@amontoison
Copy link
Contributor

@ocots

help?> isconcretetype
search: isconcretetype

  isconcretetype(T)

  Determine whether type T is a concrete type, meaning it could have direct instances (values x
  such that typeof(x) === T).

  See also: isbits, isabstracttype, issingletontype.

  Examples
  ≡≡≡≡≡≡≡≡

  julia> isconcretetype(Complex)
  false
  
  julia> isconcretetype(Complex{Float32})
  true
  
  julia> isconcretetype(Vector{Complex})
  true
  
  julia> isconcretetype(Vector{Complex{Float32}})
  true
  
  julia> isconcretetype(Union{})
  false
  
  julia> isconcretetype(Union{Int,String})
  false

Union{...} is not a concrete type, which is why I recommended adding more parameters to your structure.

@ocots
Copy link
Member Author

ocots commented Sep 10, 2024

Thanks. But our model can be constructed incrementally and it may not be so costly since it is transcribed when we solve it. But the callable type are called many times so I think this is more crucial. I am updating these types.

@ocots ocots linked a pull request Sep 10, 2024 that will close this issue
@ocots
Copy link
Member Author

ocots commented Sep 11, 2024

@ocots Replace

function BoundaryConstraint(f::Function, dependencies::DataType...)

by

function BoundaryConstraint(f::Function, VD::Type{<:VariableDependence})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal dev Modification to the code is requested and should not affect user experiment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants