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

Automatic field type conversions in types #174

Open
kylebeggs opened this issue Jan 15, 2025 · 3 comments
Open

Automatic field type conversions in types #174

kylebeggs opened this issue Jan 15, 2025 · 3 comments
Labels
architecture Issues affecting the structural and design aspects of the software

Comments

@kylebeggs
Copy link
Collaborator

Some types make use of Base.@kwdef and that is usually nice, but sometimes it can be a pain for the user to make sure all the fields are the correct (same) type because they are strictly parametrized so. I'm wondering if it would be better to simplify things a bit by automatically converting those types. To make my proposal more concrete, my main example is

Base.@kwdef struct AdaptiveForwardEulerSubstepper{T, SolutionVectorType <: AbstractVector{T}} <: AbstractPointwiseSolver
substeps::Int = 10
reaction_threshold::T = 0.1
solution_vector_type::Type{SolutionVectorType} = Vector{Float64}
batch_size_hint::Int = 32
end

I think the intention is whatever you set solution_vector_type to, thats what the other fields (just reaction_threshold in this case) are intended to be. I suggest something like:

struct AdaptiveForwardEulerSubstepper{T,SolutionVectorType<:AbstractVector{T}} <: AbstractPointwiseSolver
    substeps::Int
    reaction_threshold::T
    solution_vector_type::Type{SolutionVectorType}
    batch_size_hint::Int
    function AdaptiveForwardEulerSubstepper(; substeps::Int=10, reaction_threshold=0.1, solution_vector_type::Type{<:AbstractVector{T}}=Vector{Float64}, batch_size_hint::Int=32) where {T}
        new{T,solution_vector_type}(substeps, T(reaction_threshold), solution_vector_type, batch_size_hint)
    end
end

or even better, but would be an API change is to propagate the type of u0 all the way here and completely elide the need to specify the solution vector type.

@termi-official
Copy link
Owner

Definitely agree that it is super painful right now.

I have a slightly different solution in mind tho. We should bundle the used types in some strategy stucts and propagate a device type, similar to KernelAbstractions.jl. xref #163 where we started working on the device types.

[...] API change is to propagate the type of u0 all the way here [...]

This is intenionally not done. Significant part of the API aims at supporting multi-device solvers. Think e.g. about solving the mechanics on the CPU, but the electrophysiology on the GPU.

Ultimately this is the same goal. I want to remove the explicit vector and matrix types in the solver types and make the initialization via the device+strategy pairs. This has for example the advantage that we can more easily ensure that we are using Int32 and Float32 consistently in the internals, anc convert invalid inputs automatically.

@termi-official termi-official added the architecture Issues affecting the structural and design aspects of the software label Jan 16, 2025
@kylebeggs
Copy link
Collaborator Author

Ok, that makes sense! I'll give that linked issue a look. In the meantime, what do we do about this then? I think close it given your future solution will fix this?

@termi-official
Copy link
Owner

Let us keep the issue open, in case we might want to discuss some alternative architectures. If you have a better idea I am always open for suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Issues affecting the structural and design aspects of the software
Projects
None yet
Development

No branches or pull requests

2 participants