A matter of taste

New to Julia
1

Hello dears,

Would you please review the herebelow piece of code w.r.t the style, the way of doing things, performance-wise if possible… not necessarily task specific, but as a Julia programmer in general, how do you feel about this code, what would be your advice if any.

Thank you.

piece of code

  function _evaluate(
                     ::HullWhite1F,
                     ::Type{Swaption},
                     t::T,
                     strike::R1,
                     expiry::T,
                     inception::T,
                     paydates::AbstractVector{T},
                     cvgs::AbstractVector{<:Real},
                     r::R2,
                     yc::YieldCurve,
                     θ::R3,
                     σ::R4,
                     ispayer::Bool=true,
                     notional::Real=1,
                     dcc::DayCountConventions=ActAct(),
                     f::Function=getcurve(yc,t,what=:forwardcurve,dcc=dcc),
                     low::R5=0.0,
                     upp::R5=5.0
           ) where {T<:TimePoint,R1<:Real,R2<:Real,R3<:Real,R4<:Real,R5<:Real}
    @assert isequal(length(paydates),length(cvgs))
    _r=_find_hw1f_rstar(paydates,cvgs,strike,yc,expiry,t,θ,σ,dcc,f,dcc,low,upp)
    xi = map(paydates) do x
      _dummy_hw1f_zcb_price(
                            yc, x, expiry, θ, σ, _r, first(gettimes(yc)),
                            dcc,notional,f,low,upp
      )
    end
    ci = strike * cvgs
    ci[end] += 1
    ocol = ifelse(ispayer,Put,Call)
    _pee = map(ci, paydates, xi) do a,b,c
      a * _evaluate(
                    HullWhite1F(), EuropeanOption{ZeroBond}, t, c, expiry,
                    ocol, b, inception, 1, r, θ, σ, yc, dcc
          )
    end
    notional * sum(_pee)
  end
1 Like
2

As you correctly point out in the title, code formatting is almost entirely a matter of taste. The important part is that you use a consistent style and apply it automatically in your CI.

Please use JuliaFormatter (or Runic, if you prefer). See my preferred .JuliaFormatter.toml file, but you can use whatever settings you like.

Personally, I’m not a fan of the hanging indent you’re using here. And four spaces per indentation level is probably pretty universal. But again, what matters is consistency and automation.

3 Likes
3

Apart from the formatting, I would say you have too many positional arguments, and you’re underusing keyword arguments. At the very least, all the arguments with default values would probably be better as keyword arguments. The general guideline is to use positional arguments for telling the function what to do, and keyword arguments for how to do it. I’d consider having more than 5 or so positional arguments to be a code smell. Keyword arguments are generally less error-prone and somewhat self-documenting. There’s also the possibility of grouping arguments in a more meaningful struct, and passing that.

Also: does it really make sense to have 5 different flavors or Reals in your parameters? You often don’t need type specifications for function arguments at all. They’re important for the performance of types, but in functions, they really only matter to identify methods in multiple dispatch. Method specialization is independent of type annotations, so there aren’t generally performance implications with functions.

6 Likes
4

I can add a few comments.

  • Most of the function parameterisations have no effect e.g. R1<:Real,R2<:Real,R3<:Real,R4<:Real. You could just add the type to the input argument strike::Real. The R5<:Real have the effect that low and upp must be the same type. Is this really needed. Also T<:TimePoint only has an effect if TimePoint is an abstract type so there is multiple options for T
  • _evaluate(::HullWhite1F,::Type{Swaption} ... have you considered dropping these arguments and just giving the function a descriptive unique name instead. The pattern you use now can be very powerful and allow you to make generic code that works for multiple models but its adds complexity and will probably not work when you have so many positional arguments.
2 Likes
5

I mostly agree with the comments from @goerz and @s153283, except that I do not agree that type specifiers only are useful to help with multiple dispatch. In my opinion they do also help avoid bugs from incorrect use of the function (e.g. specifying arguments in the wrong order) and they also have a self documenting effect. So I feel that this can be acceptable in personal code and if done with care also in public code.

Following up on the recommendation to be consistent, it is also most common to have spaces after commas and your code is not consistent about that.

Finally, be aware that the extent of acceptable use of non-ASCII characters in Julia varies from person to person and many (but not Julia itself) completely avoid using it for variable names. So if you plan on making your code very public, this may be important for you also.

2 Likes
6

It’s not as simple as generally moving positional arguments to keyword arguments, though. A dozen non-optional keyword arguments is just as unwieldy, and the ability to write them in an arbitrary order doesn’t make up for the extra writing of keywords or sacrificing method dispatch. Keywords are much more useful for optional arguments with default values, which tend to lean towards “how” (options) instead of “what” (data).

Grouping arguments into a composite type also has to be done on a case by case basis. If those arguments don’t form a cohesive group across various function calls, even roughly, then throwing them into a composite type right before function calls and needing to disassemble it right after has little benefit. A complete lack of cohesion across so many arguments is just rare enough that one should first suspect otherwise. It’s hard to tell because it’s not a MWE, but it does appear that many unchanged arguments are passed to callees and could be grouped together.

The inner _evaluate call appears to take arguments in a different order, implying there’s another method. If that’s not supposed to be a recursive method, it shouldn’t be the same function so there’s no risk of edits accidentally causing recursion.

3 Likes
7

I actually agree with all of that! It’s hard to make general statements. :slight_smile:

3 Likes
8

Thank you for your reply.
…the hanging indent is to ease reviewing process…
About the universal use of 4 spaces, I’ve never been a fan of it. I constraint myself to have 80-chars width scripts (since sometimes I fix codes using nano thru an OS not having the GUI installed) so wasting 2 spaces is a lot for me.

9

Yes, too many positional arguments I do agree. But most of the time functions I do expose to users have kwargs built in an ergonomic way (at least I try my best). What I mean is that I’ll built _evaluate as an internal method but the user will interact with a nice ergonomic evaluate having much less positional args.

About type annotations yes, I sometimes go to much on it yes. But I really do need type annotations, I have overloaded functions like Base.:+ … to built some additive monoids/groups so I need some methods to fail for some input.

But yes, you’re right I overuse multiple dispatch like a baby having a new toy… I’ll refine my understanding of it to use it the proper way, thank you.

10

Thank you sir for your reply.
Yes, T<:TimePoint is a abstract (parametric) type.
Btw, I had some failures in the past when doing param::Real, even recently I experienced a weird failure with arg::Real (I mean the behavior was not what I wanted… I don’t clearly remember the example to show a MWE…). To be honest for low and upp I could’ve done low::Real (Float64 would fit but I experienced some bad errors with ForwardDiff when restricting types to Float64…).

About your second point, I try not to have too many names in the package that’s why I go wild on multiple dispatch but perhaps I overuse it, indeed. I should refine my understanding in order to use it the proper way.

1 Like
11

Yes sir,
…type annotations to be very strict (like in C) on the inputs and yeah avoiding bugs, bad usage, black-hat injections… and most of the time multiple dispatch (foo(::Int64,::Float64) = "GHTaarn", foo(::Float64,::Int64)="goerz").

Thank you for your comment.

1 Like
12

Thank you @Benny for your comments, I appreciate them.

13

I’m surprised that you find it helping with code review. Hanging indents obscure the logical indent level, so I would think that they make review harder, not easier. But you do you.

That’s going to complicate you interacting with other people’s code, and vice versa. All the more important to have automatic formatting in place. And you should also add an .editorconfig to your project if you’re using extremely non-standard formatting like that.

So do I, working in tmux and vim almost exclusively. Although Julia’s syntax can make it hard to stick to a strict 80 character limit. I use 92 as a pragmatic choice for my automatic code formatter. That allows to stay below 80 for almost all lines, but then have the occasional long line when necessary.

That makes the hanging indent even more baffling, because that’s a massive waste of horizontal space. Just to illustrate the point, here is your code reformatted with normal indent and 4-space indentation:

function _evaluate(
    ::HullWhite1F,
    ::Type{Swaption},
    t::T,
    strike::R1,
    expiry::T,
    inception::T,
    paydates::AbstractVector{T},
    cvgs::AbstractVector{<:Real},
    r::R2,
    yc::YieldCurve,
    θ::R3,
    σ::R4,
    ispayer::Bool = true,
    notional::Real = 1,
    dcc::DayCountConventions = ActAct(),
    f::Function = getcurve(yc, t, what = :forwardcurve, dcc = dcc),
    low::R5 = 0.0,
    upp::R5 = 5.0
) where {T<:TimePoint,R1<:Real,R2<:Real,R3<:Real,R4<:Real,R5<:Real}
    @assert isequal(length(paydates), length(cvgs))
    _r = _find_hw1f_rstar(
        paydates, cvgs, strike, yc, expiry, t, θ, σ, dcc, f, dcc, low, upp
    )
    xi = map(paydates) do x
        _dummy_hw1f_zcb_price(
            yc, x, expiry, θ, σ, _r, first(gettimes(yc)), dcc, notional, f,
            low, upp
        )
    end
    ci = strike * cvgs
    ci[end] += 1
    ocol = ifelse(ispayer, Put, Call)
    _pee = map(ci, paydates, xi) do a, b, c
        a * _evaluate(
            HullWhite1F(), EuropeanOption{ZeroBond}, t, c, expiry, ocol, b,
            inception, 1, r, θ, σ, yc, dcc
        )
    end
    notional * sum(_pee)
end

That actually uses less horizontal space, and is two lines shorter than your original code. And, it would not have made any difference whatsoever if I had used two-space indentation.

To be fair, I’ve edited the above code to allow for more than a single argument per line in function calls (like you have in the original code). The more usual one-argument-per-line would of course increase the number of lines, but make for easier editability and cleaner diffs. It would also make the difference between a two-space vs. four-space indent even less significant.

With the more commonly used code formatting schemes, there is no benefit to reducing the number of spaces per indent level, until you get to an extreme level of nesting. And probably, nesting more than 6 or so levels is a code smell in itself.

14

haha, thank you!
Definitely a matter of taste…
I do this vertical alignment whenever I can’t do one-line for all the args. To be honest, I find easier and more clearer to check the following code than any other option (at least when defining the function).

 function foo(
              arg1::T1,
              arg2::T2,
                   ...
              argn::Tn
          )
  ...lottalines...
 end

I’ll quickly process arg1::T1, …, argn::Tn in my head…

15

twk
twk673×357 47 KB

… look for egz…

16

I see this as one more reason to replace the _evaluate methods with functions with descriptive names. I don’t think you will face any issues with the number of function names. Plus you can use the public evaluate function to dispatch on different model types.

1 Like
17

Yes, I dispatch with evaluate too, but a lil bit less…
I avoid too many names because I often do PackageName.+\TAB when I’m in the REPL to have a glance of the API… having too many names is kinda boring!

18

Yes but on the other extreme. PackageName.+\TAB is useless if everything is just one function name with a ton of multiple dispatch methods.

19

smilz! Got you… need to find the right balance!

1 Like