UI for new homogenization function: "confusion" between optional parameter and keyword parameter

[JohnAAbbott]

I have just spent some time tracking down a "weird, syntactic bug": I made a minor "almost invisible" typo, and obtained unexpected results. My question will be: should we change the UI so that others cannot make the same mistake?

function homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName; extra_gens_flag::Bool = false) where {T <: MPolyRingElem}
...
function homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName, pos::Int; extra_gens_flag::Bool = false) where {T <: MPolyRingElem}

These two methods correspond to the last parameter pos being optional (otherwise the signatures are the same). The bug was triggered by an incorrect call, namely:

homogenization(I, W, h, extra_gens_flag=extra_gens_flag)

The crucial point is that I typed a comma instead of a semicolon (just after h). This call was accepted by Julia (without warnings), but it matched the second signature: the boolean value of extra_gens_flag was automatically coerced into an Int. With a semicolon instead of a comma the call matches the first signature (and does what I'd expected/intended).

Yes, there are several could have and should have (but I didn't). My main point is that it would be easy for another person to make the same mistake... and I can assure you it was not easy to spot the typo!

So, how could I revise the UI (i.e. the signatures) so that this minor typo cannot cause future headaches?
My suggestion would be to change pos into a keyword parameter; this means that calls to homogenization where the value of pos is to be specified would look like the first two in the following:

homogenization(J, W, h; pos=1)
homogenization(J, W, h; pos=1, extra_gens_flag=true)
homogenization(J, W, h; extra_gens_flag=true)  # use default value of pos

My description here is (obviously) in the context of the homogenization function, but the message is intended to be general.

[JohnAAbbott]

This comment is more "private" (i.e. relevant only to those interested in the function homogenization).
@wdecker What do you think of changing pos into a keyword parameter? I like the idea because a call to homogenization with a nameless integer argument is not easy to understand (unless one has the signature clearly in memory). Taking the example from above:

homogenization(J, W, h; pos=1)

seems to be easier to read/comprehend than the current form

homogenization(J, W, h, 1)

where a reader unfamiliar with the signature would like wonder what the argument 1 means in this context.
Yes, I know that my proposed change is not backwards compatible (and would entail rewriting some documentation). But I currently think that it brings genuine advantages: e.g. easier to read/comprehend, less risk of problems from an "almost invisible" typo.

[wdecker]

I agree.

…

Am 20.07.2023 um 14:31 schrieb JohnAAbbott ***@***.***>: This comment is more "private" (i.e. relevant only to those interested in the function homogenization). @wdecker <https://github.com/wdecker> What do you think of changing pos into a keyword parameter? I like the idea because a call to homogenization with a nameless integer argument is not easy to understand (unless one has the signature clearly in memory). Taking the example from above: homogenization(J, W, h; pos=1) seems to be easier to read/comprehend than the current form homogenization(J, W, h, 1) where a reader unfamiliar with the signature would like wonder what the argument 1 means in this context. Yes, I know that my proposed change is not backwards compatible (and would entail rewriting some documentation). But I currently think that it brings genuine advantages: e.g. easier to read/comprehend, less risk of problems from an "almost invisible" typo. — Reply to this email directly, view it on GitHub <#2582 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASSXESMKSTETLQ7NR4SJATXREQJ5ANCNFSM6AAAAAA2RKYGYE>. You are receiving this because you were mentioned.

[JohnAAbbott]

I have just pushed revised code which (hopefully) implements this revised UI. A few tests on my machine suggested all is OK.
I have revised some of the documentation, but must still look at any tests (and .md files).

[JohnAAbbott]

Do we have guidance for writing documentation for functions with optional keyword arguments?
The specific case I have in mind is homogenization (for ideals, but analogous questions arise for other variants of homogenization). depending on how they are counted there are up to 8 cases. The most compact description is:

homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName; pos::Union{Int,Nothing} = nothing, extra_gens_flag::Bool = false)
homogenization(I::MPolyIdeal{T}, h::VarName; pos::Union{Int,Nothing} = nothing, ordering::MonomialOrdering = default_ordering(base_ring(I)))

If we expand out each possible case we obtain this rather long version:

homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName; pos::Union{Int,Nothing} = nothing, extra_gens_flag::Bool = false)
homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName; pos::Union{Int,Nothing} = nothing)
homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName; extra_gens_flag::Bool = false)
homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName)
homogenization(I::MPolyIdeal{T}, h::VarName; pos::Union{Int,Nothing} = nothing, ordering::MonomialOrdering = default_ordering(base_ring(I)))
homogenization(I::MPolyIdeal{T}, h::VarName; pos::Union{Int,Nothing} = nothing)
homogenization(I::MPolyIdeal{T}, h::VarName; ordering::MonomialOrdering = default_ordering(base_ring(I)))
homogenization(I::MPolyIdeal{T}, h::VarName)

Or we could employ a verbal description, perhaps something like the following

homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName)
with two optional kwargs "pos" and "extra_gens_flag" (...more description...)
homogenization(I::MPolyIdeal{T}, h::VarName)
with two optional kwarg "pos" and "ordering"  (... more description...)

In fact the normal argument W is in a sense optional (but this cannot be expressed directly using julia syntax); so the verbal description of two functions above could be shortened to just one:

homogenization(I::MPolyIdeal{T}, W::Union{ZZMatrix, Matrix{<:IntegerUnion}}, h::VarName)
where the parameter "W" may be omitted (in which case is [1 1 1 ...] is assumed), and
with two optional kwarg "pos" and "ordering" ...

Which approach is easiest to comprehend? Or should we just use the first option (i.e. julia syntax), and then give 8 examples to show the different variations?

[JohnAAbbott]

Since one revision to the user interface to homogenization has already been made, it is perhaps a good time to revisit the entire interface. I'd say that I am mostly happy with the current proposal (e.g. as in my lengthy post just above). I do wonder whether the VarName parameter should also be optional; if so, we need to choose a default name (I'd suggest "h").
The current UI requires the caller to choose a name even if the caller thinks "don't care".
If we do make the name optional then it would be possible to have calls such as the following:

homogenization(f)
homogenization(I)
homogenization(f, W)
homogenization(I, W)

If the parameter is an optional kwarg then specifying the name would require writing something like this:

homogenization(f;    h_name="x[0]")
homogenization(I;    h_name="x[0]")
homogenization(f, W; h_name="h")
homogenization(I, W; h_name="h")

The kwarg name h_name is just ad hoc for illustration -- other proposals are welcome.
Is it a reasonable/good idea to:
(A) make the homogenizing variable name optional?
(B) specify the name via a kwarg rather than a normal arg?

Bear in mind that if we wish to specify both name and position then we must write something like:

homogenization(f; h_name="x[0]", pos=1)
homogenization(I; h_name="x[0]", pos=1)
homogenization(f, W; pos=1, h_name="h")
homogenization(I, W; pos=1, h_name="h")

This is starting to become rather "verbose"...
Anyway, I would like to solicit ideas/opinions about how to improve the UI for homogenization