`HModuleElt` constructor relies on `Pair`'s questionable promotion behavior

[adienes]

in JuliaLang/julia#58836, triage desires to remove the promote_rule defined for Pair (and Some) as to make it consistent with every other container type in the language

I am going through the results of the Nanosoldier run so as to avoid as much potential downstream impact as possible.

I noticed that the line here

ModuleElts.jl/src/ModuleElts.jl

Line 189 in 6915aec

function HModuleElt(x::Vector{Pair{K,V}};check=true) where{K,V}

relies on Base.vect to call promote_typeof on the Pairs, promoting them to a common concrete K, V

would it be feasible to loosen that signature? I believe the Dict constructor should be trying to handle promotion and {K,V} type anyway.

[jmichel7]

I am not sure I understand your point or conversely that you understand how ModuleElt is designed.

ModuleElt implements two types: HModuleElt, to show that the functionality can be (and is usually) implement by Dicts, but
the main point is the type ModuleElt which is implemented by using sorting instead of hashing and is up to 10 times faster in practice. So I don't really care about the type HModuleElt, but care about ModuleElt. So the line which worries me is
line 228.

function ModuleElt(d::AbstractVector{Pair{K,V}};check::Bool=true)where `{K,V}`

Do you say that the pairs won't be any more promoted and I will have to write complicated code to do that?
Will Dicts constructors have to write also new code? Or do I understand wrongly?

This would be acceptable is this new code is packaged as an easy-to-call function. Or if you write the code for me. Otherwise
I would claim too disruptive for julia 1.x

[adienes]

I am not sure that you understand how ModuleElt is designed.

I think you are certainly right :)

Dict constructors would not be affected and will not write new code. it is correct that pairs would no longer promote their first, second parameters independently, so their promotion behavior would be consistent with every other container type in the language. note that the promotion actually happens from Base.vect and not anywhere inside Dict (nor ModuleElt)

so it was a test that called something like ModuleElt([:xy=>1//2,:yx=>-1]) that was failing. on nightly,[:xy=>1//2,:yx=>-1] isa Vector{Pair{Symbol, Rational{Int64}}} whereas after JuliaLang/julia#58836 it would be a Vector{Pair{Symbol}}

certainly the hope is that such a change is not too disruptive! there were only a small number of PkgEval flagged packages (~4 active) hence why I am trying to be proactive within each package to work towards a resolution

I will have to write complicated code to do that?

in this case, I am not sure but I think simply deleting {K, V} could resolve the issue, so if anything the new code will be simpler!

[jmichel7]

it was a test that called something like ModuleElt([:xy=>1//2,:yx=>-1]) that was failing. on nightly,

I was afraid of that. This will cause many failures where I use ModuleElts. I note that similarly, we currently have

julia> Dict([:xy=>1//2,:yx=>-1])
Dict{Symbol, Rational{Int64}} with 2 entries:
:xy => 1//2
:yx => -1

Are you not afraid that changing this will cause many failures (even though is maybe not specifically tested)?

Also

simply deleting {K, V} could resolve the issue

ModuleElts need to have a type {K,V}. They represent e. g. elements of an algebra with type of basis K and type of coefficients V. It would be very bad to lose either of the types...

[adienes]

just to clarify:

julia> Dict([:xy=>1//2,:yx=>-1])
Dict{Symbol, Real} with 2 entries:
  :xy => 1//2
  :yx => -1

is due to the fact that [:xy=>1//2,:yx=>-1] is a Vector{Pair{Symbol, Rational{Int64}}} and has nothing to do with Dict. the PR would mean that [:xy=>1//2,:yx=>-1] is a Vector{Pair{Symbol}} so that Dict(x) has consistent types with Dict(x...)

also to this question

Are you not afraid that changing this will cause many failures

of course that was a concern of triage, hence why I ran PkgEval, read through the failures, and opened issues in the relevant packages. if it were not a concern, it would simply be merged...

even though as you note this promote behavior of Pair is not specifically documented or tested, so it is technically not public API and is subject to change in minor version releases, of course in practice breaking people's code is not a great way to make friends 🙂. also, this is not particularly "my" change. I am just a volunteer who currently happens to be the one trying to shepherd the PR along, but there are several stakeholders who wish to change promote_type(::Pair, ::Pair). if you have strong thoughts to the contrary I recommend voicing them in the discussion there

back to the main topic, perhaps something like adding a constructor could suffice? I tried this and it passed all your package tests locally

struct ModuleElt{K,V} <: AbstractDict{K,V}
  d::Vector{Pair{K,V}}
  function ModuleElt{K,V}(d::AbstractVector{Pair{K,V}}; check::Bool=true) where {K,V}
    if check normalize!(d) end
    new{K,V}(d)
  end
end

function ModuleElt(d::AbstractVector{Pair{K,V}}; check::Bool=true) where {K,V} 
  ModuleElt{K,V}(d; check)
end

function ModuleElt(d::AbstractVector{<:Pair}; check::Bool=true)
  K = mapreduce(typeof ∘ first, Base.promote_type, d)
  V = mapreduce(typeof ∘ last, Base.promote_type, d)
  ModuleElt{K,V}(Vector{Pair{K,V}}(d); check)
end

so

• when a Vector{Pair{K,V}} for concrete K,V is explicitly given, use them directly.
• otherwise, grab a concrete K,V from the typeof each item and recreate the promote_type that Pair on master currently promotes like.

[jmichel7]

perhaps something like adding a constructor could suffice?

Thank you! This is what I had in mind when I wrote "write the code for me". In other words, your solution looks feasible to me. I will test it and if it works make a PR to my repository which will hopefully settle the matter.