sciml: double indent for function and macro definitions

[domluna]

No description provided.

[ChrisRackauckas]

@isaacsas can you test this to see if it covers everything you were interested in?

[isaacsas]

I thought this was the latest formatting given by this PR per the comments in the open issue, which is the wrong style and seems to be adding extra lines when the text is under the col character size limit.

function get_num_majumps(
    smaj::SpatialMassActionJump{Nothing, B, S, U, V},
) where
    {B, S, U, V}
        size(smaj.spatial_rates, 1)
end

When I’d expect

function get_num_majumps(
        smaj::SpatialMassActionJump{Nothing, B, S, U, V}) where {B, S, U, V}
    size(smaj.spatial_rates, 1)
end

[domluna]

i haven't updated it to indent the function args by 2x instead. also the where is on a different line because that's how it is in the source file and join_lines_based_on_source = true. if you lift up the where to the same line in the original source it will be on the same line.

[isaacsas]

I’m happy to test it out once you’ve updated the indentation. Thanks!

[domluna]

@isaacsas give it a shot now

[isaacsas]

Traveling today, but will check when I get home. Thanks!

[isaacsas]

Is this supposed to change function call formatting too like follows? Original:

jprob = JumpProblem(dprob, Direct(), maj,
                    rng = Xorshifts.Xoroshiro128Star(rand(UInt64)))

reformats to

jprob = JumpProblem(dprob, Direct(), maj,
    rng = Xorshifts.Xoroshiro128Star(rand(UInt64)))

The reformatted version, to me, looks like someone is trying to assign to a variable called rng? I don't believe SciMLStyle used to reformat like this?

[isaacsas]

Another one:

function urate2(u, p, t)
    if u[1] > 0
        1e-2 * max(u[4],
            (u[4] * exp(-1 * u[1] / 1e5) +
             (u[2] * u[3] / u[1]) * (1 - exp(-1 * u[1] / 1e5))))
    else
        1e-2 * (u[4] + 1 * u[2] * u[3] / 1e5)
    end
end

becomes

function urate2(u, p, t)
    if u[1] > 0
        1e-2 * max(u[4],
            (
                u[4] * exp(-1 * u[1] / 1e5) +
                (u[2] * u[3] / u[1]) * (1 - exp(-1 * u[1] / 1e5))
            ))
    else
        1e-2 * (u[4] + 1 * u[2] * u[3] / 1e5)
    end
end

[isaacsas]

@inline function FastBroadcast.fast_materialize!(::FastBroadcast.True, ::DB, dst::EJA,
                                                 bc::Base.Broadcast.Broadcasted{S}) where {
                                                                                           S,
                                                                                           DB,
                                                                                           EJA <:
                                                                                           ExtendedJumpArray
                                                                                           }
    FastBroadcast.fast_materialize!(FastBroadcast.True(), DB(), dst.u,
                                    JumpProcesses.repack(bc, Val(:u)))
    FastBroadcast.fast_materialize!(FastBroadcast.True(), DB(), dst.jump_u,
                                    JumpProcesses.repack(bc, Val(:jump_u)))
    dst
end

becomes

@inline function FastBroadcast.fast_materialize!(::FastBroadcast.True, ::DB, dst::EJA,
        bc::Base.Broadcast.Broadcasted{S}) where {
    S,
    DB,
    EJA <:
    ExtendedJumpArray,
}
    FastBroadcast.fast_materialize!(FastBroadcast.True(), DB(), dst.u,
        JumpProcesses.repack(bc, Val(:u)))
    FastBroadcast.fast_materialize!(FastBroadcast.True(), DB(), dst.jump_u,
        JumpProcesses.repack(bc, Val(:jump_u)))
    dst
end

which doesn't look like the correct style again.

[isaacsas]

function Base.BroadcastStyle(::Type{ExtendedJumpArray{T3, T1, UType, JumpUType}}) where {T3,
                                                                                         T1,
                                                                                         UType,
                                                                                         JumpUType
                                                                                         }
    ExtendedJumpArrayStyle(Base.BroadcastStyle(UType), Base.BroadcastStyle(JumpUType))
end

becomes

function Base.BroadcastStyle(
        ::Type{ExtendedJumpArray{T3, T1, UType, JumpUType}},
) where {T3,
    T1,
    UType,
    JumpUType,
}
    ExtendedJumpArrayStyle(Base.BroadcastStyle(UType), Base.BroadcastStyle(JumpUType))
end

[isaacsas]

So it looks like there are still some issues.

[isaacsas]

function SpatialMassActionJump(urates::A, srates::B, rs::S, ns::U, pmapper::V;
                               scale_rates = true, useiszero = true,
                               nocopy = false) where {A <: AVecOrNothing,
                                                      B <: AMatOrNothing, S, U, V}
    SpatialMassActionJump{A, B, S, U, V}(urates, srates, rs, ns, pmapper, scale_rates,
                                         useiszero, nocopy)
end

becomes

function SpatialMassActionJump(urates::A, srates::B, rs, ns; scale_rates = true,
        useiszero = true,
        nocopy = false) where {A <: AVecOrNothing,
    B <: AMatOrNothing}
    SpatialMassActionJump(urates, srates, rs, ns, nothing; scale_rates = scale_rates,
        useiszero = useiszero, nocopy = nocopy)
end

[isaacsas]

@ChrisRackauckas perhaps we can just go with @YingboMa's original suggestion to roll back the last style change? I don't think we are heading in an improved direction for readability currently. The previous version was much more flexible in preserving the formatting choice of a user which, at the end of the day, is likely to be more readable than a fixed, forced rule. The many examples above illustrate this. Wrapping if function text gets too long to fit the col width makes sense, but currently wrapping and the creation of new lines seem to occur for very readable code that is within the col width to enforce alignment of arguments at a fixed number of indented characters, which in many cases seems to me to reduce readability and lead to issues like I've posted above.

[domluna]

sciml style is really difficult to pin down, it seems within it there are several parties that have conflicting ideas about indentation and so we get this mix that produces sometime seemingly odd results.

[ChrisRackauckas]

Is there? From what I can read there was a single voted on resolution to #741, and it's #741 (comment), with an additional line space added there, i.e.:

function alg_cache(alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
        ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
        dt, reltol, p, calck, ::Val{true}) where {uEltypeNoUnits, 
        uBottomEltypeNoUnits, tTypeNoUnits}

    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

It's by far the most upvoted over and over, I put the extra space in there so it handles the complaints of it being too compact, so let's settle on this and people can flip other options around how they want.

[domluna]

#776 should be the minimal change required for this

[domluna]

closing in favour of #776