Homogenization: the pos parameter?

[JohnAAbbott]

The functions homogenization for polynomials and ideals have a pos parameter (with default value 1).
Is this parameter actually used anywhere: I mean, when might one call homogenization and specify a non-default value for this parameter?
I ask because my current new implementation for homogenization (of ideals) does not yet have such a parameter. It could be added, but would make the code rather messier. So I just want to be convinced that there are genuine use-cases where the caller needs to specify the value for pos. I would not expect the extra code needed to handle a pos value to have a measurable adverse effect on execution speed... readability is another matter altogether.

[wdecker]

This is naturally used EVERYWHERE. Think, for example, about going back and force from a projective variety in P^n to the corresponding affine varieties in affine charts U_0, …, U_n. We may consider, however, to make the default value n+1. This will ease the problems arising from the fact that vectors in julia are not allowed to start from index 0. In OSCAR, we are forced to use 1, …, n+1 instead.

…

Am 27.06.2023 um 13:04 schrieb JohnAAbbott ***@***.***>: The functions homogenization for polynomials and ideals have a pos parameter (with default value 1). Is this parameter actually used anywhere: I mean, when might one call homogenization and specify a non-default value for this parameter? I ask because my current new implementation for homogenization (of ideals) does not yet have such a parameter. It could be added, but would make the code rather messier. So I just want to be convinced that there are genuine use-cases where the caller needs to specify the value for pos. I would not expect the extra code needed to handle a pos value to have a measurable adverse effect on execution speed... readability is another matter altogether. — Reply to this email directly, view it on GitHub <#2494>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASSXEUJNBHSVEFTZ3RK2XTXNK42PANCNFSM6AAAAAAZVNWDKY>. You are receiving this because you are subscribed to this thread.

[JohnAAbbott]

OK. I'll update the code.

[JohnAAbbott]

i have pushed the new code to github. The new function is called homogenization2 and is intended as a drop-in replacement for homogenization (for ideals). I have given it a different name so that it is easy to try the new and old versions side-by-side. I'll make the PR a "draft" (once I have figured out how).
I know that the newly uploaded code does not reflect the OSCAR coding standards -- I plan to fix this once the PR is no longer a draft.

[JohnAAbbott]

After talking with @wdecker yesterday we decided that the default value for the pos parameter should be 1+nvars. I am happy with this decision. But there is a small implementation snag. I would like to write:

function homogenization(V::Vector{T}, var::VarName, pos::Int = 1+ngens(parent(V[1])))

but this will crash/error if V is empty. One solution is to write this more complicated form

function homogenization(V::Vector{T}, var::VarName, pos::Int = (isempty(V) ? 1 : 1+ngens(parent(V[1]))))

which does not score too highly in the readability stakes. Another possibility is to do the following

function homogenization(V::Vector{T}, var::VarName, pos::Int = nothing)
     if isempty(V)
        #=handle empty V case-=#
    else if pos == nothing
        pos = 1+ngens(parent(V[1]))
    end

I welcome guidance about preferred approach here, ideally with some justification for the guidance.

[thofma]

Readability is not an issue. I would go with the second (complicated) form, unless you need a sentinel value to handle the empty case (could also be -1 with the second form).

In the third option, julia does not like that pos would change its type, so you would have to do some _pos = 1 + ngens(...) dance.

[JohnAAbbott]

I had hoped that the compiler would be a bit smarter than it is... the following code does not work as one might reasonably expect

function f1(n::Int)
    return j+1
end

function f2(a::Int, b::Union{Int,Nothing}=nothing)
    if b == nothing
        b = a
    end
    # at this point the compiler **should** be able to deduce that b is of type Int, but it does not
    return f1(a+b)
end

[thofma]

Is it giving a wrong result? Seems to work as expected for me.

[JohnAAbbott]

Oops! There was a typo. After correcting the typo it seemed to work. Does that mean that it did deduce that b has type Int?

[thofma]

Yes, it seems so. Not clear which compiler optimisation is responsible, but I would not rely on it.

[JohnAAbbott]

Post first, think later... I overlooked the obvious and most flexible solution: namely write out explicitly two methods:

function homogenization(V::Vector{T}, var::VarName))
    if isempty(V)  #=handle empty vector=#
    return homogenization(V, var, 1+ngens(parent(V[1])))
end
function homogenization(V::Vector{T}, var::VarName, pos::Int)
    if isempty(V)  #=handle empty vector=#

Advantages: reader can tell at-a-glance that there are 2 methods, full flexibility for computing the default value.
Disadvantages: a bit more typing (so perhaps greater chance of a typo?), must handle empty vector twice (i.e. more typing).
We don't see it here, but the shorter signature likely corresponds to a very short implementation (namely, compute default value then call the main function).

[thofma]

Yeah, I agree that this is the cleanest version.

[JohnAAbbott]

Thanks for the feedback. I shall revise the code to adopt this approach. It's also the approach I feel most comfortable about.

[JohnAAbbott]

I have now revised the code to use 2 explicit function definitions rather than a default value for the pos parameter.
I have also renamed: the new implementation is now called simply homogenization, while the old one (based on naive saturation) is called homogenization_via_saturation. At the moment I cannot think of a reason to keep the old code around, but prefer not to delete it yet. The function homogenization_via_saturation is not documented (since it will likely be deleted sooner or later).

[JohnAAbbott]

See also discussion #2582