search

JuliaSmoothOptimizers / Krylov.jl

A Julia Basket of Hand-Picked Krylov Methods
Other
349 stars 51 forks source link

`ktypeof(::AbstractVector)` is breaking between 0.9.0 and 0.9.1 #749

Closed charleskawczynski closed 1 year ago

charleskawczynski commented 1 year ago

Hello, thank you for the great package!

It appears that ktypeof is a part of the API, however, https://github.com/JuliaSmoothOptimizers/Krylov.jl/pull/705 changed the behavior of ktypeof for AbstractVector and has broken our CI based on a patch update.

I'm wondering whether it's expected that users overload ktypeof for their own types, or if this was a mistake to be reverted (so that we know how to move forward).

amontoison commented 1 year ago

Hi @charleskawczynski! Can you give an example of your AbstractVector? I can update the function ktypeof to keep the old behavior for your array type: https://github.com/JuliaSmoothOptimizers/Krylov.jl/blob/main/src/krylov_utils.jl#L212 I did it for FillArrays.jl.

charleskawczynski commented 1 year ago

So, I think we were relying on this form, however, or "FieldVector" types are similar to BlockArrays:

struct FieldVector{T, M} <: BlockArrays.AbstractBlockVector{T}
    values::M
end

so, it looks like the return type of Krylov.ktypeof has changed.

I think we can recover the previous return type easily by defining our own Krylov.ktypeof, and make it work a bit more generally (e.g., for Cu-backed arrays). But I'm now wondering if we should instead define OurArrayType(undef) and OurArrayType(undef, ::Int) instead?

amontoison commented 1 year ago

I opened #750 to keep the old behaviour of ktypeof for your type FillVector. What is the name of your package such that I can test my hotfix?

charleskawczynski commented 1 year ago

The package is https://github.com/CliMA/ClimaCore.jl, but your hot fix will not apply to CuArrays, which we need. I have a solution on our side:

import ClimaComms

# https://github.com/JuliaSmoothOptimizers/Krylov.jl/issues/749
function Krylov.ktypeof(v::S) where {S <: Fields.FieldVector}
    return ClimaComms.array_type(v){eltype(parent(v))}
end

ClimaComms.array_type(x::Fields.FieldVector) = _array_type(x)

@inline _array_type(x::Fields.FieldVector) = _array_type(x, propertynames(x))
@inline _array_type(x::Fields.FieldVector, pns::Tuple{}) = Any

@inline _array_type(x::Fields.Field) = ClimaComms.array_type(x)
@inline _array_type(x::Fields.FieldVector, sym::Symbol) =
    _array_type(getproperty(x, sym))

@inline _array_type(x::Fields.FieldVector, pns::Tuple{Symbol}) =
    _array_type(getproperty(x, first(pns)))

@inline _array_type(x::Fields.FieldVector, pns::Tuple) = promote_type(
    _array_type(getproperty(x, first(pns))),
    _array_type(x, Base.tail(pns))...,
)

But I think this is pretty specific to our code

charleskawczynski commented 1 year ago

After reviewing things more, I think that we are calling ktypeof ourselves, and we can just implement this on our end. I'll re-open if that's not the case, but if it is, sorry for the noise!

amontoison commented 1 year ago

No problem @charleskawczynski If you need a modification for your package here, don't hesitate to open a new issue. :slightly_smiling_face: