Skip to content

Merge BijectiveDicts into Bijections#15

Merged
scheinerman merged 1 commit intoJuliaCollections:masterfrom
mofeing:merge-bijectivedicts
May 26, 2025
Merged

Merge BijectiveDicts into Bijections#15
scheinerman merged 1 commit intoJuliaCollections:masterfrom
mofeing:merge-bijectivedicts

Conversation

@mofeing
Copy link
Collaborator

@mofeing mofeing commented May 26, 2025

Following discussion in JuliaRegistries/General#131059, I'm merging the modifications made in bsc-quantic/BijectiveDicts.jl in here.

I've tried to be as less disruptive as posible, but I've also have taken some freedom to update some parts of the code to minimize overhead and be more idiomatic.

Some of these changes include:

  • Stop explicitly exporting functions already in Base They are already present in any other module.
  • Parameterize Bijection's underneath dictionary types
  • Remove show method It's already handled by AbstractDict. I guess this was from a time where show was not implemented for AbstractDict, but since LTS version is Julia 1.10 and this will be a breaking release, I guess it's okay to assume that all users wil be using a Julia version that supports it.
  • Explicitly implement haskey, keys and values This gives the opportunity to be faster as if not implemented, it will probably need to iterate to reconstruct each function.
  • Introduce hasvalue function This behaves just like haskey but for the keys of the inverse mapping. It actually calls haskey on finv, which should be much faster than iterating in some cases.
  • Allow updating a value on setindex! when there is an existing key (but the value is not present) This is a common pattern I find myself in and that it's useful. I previously implemented into some coherence bugs, so this implementation avoids errors by checking that the value is not present.
  • Simplify inv and active_inv The way inv was implemented was no longer valid.
  • Do nothing (not error) if the key is not present on delete!
  • Implement sizehint!
  • More tests for edge cases...

@scheinerman scheinerman merged commit 2cd3eb9 into JuliaCollections:master May 26, 2025
1 of 4 checks passed
@scheinerman
Copy link
Collaborator

scheinerman commented May 26, 2025

Thank you @mofeing . This was a lot that you did! I'll look it over. I see you've added a lot of tests! Do you think it's nearly ready for release?

@mofeing
Copy link
Collaborator Author

mofeing commented May 26, 2025

Thank you for accepting the changes so quick! I already have been working with BijectiveDicts.jl for a while so I already knew which were the edge cases and how to test them.

Do you think it's nearly ready for release?

I think so. We should update the README with a "breaking notice" to tell users that the Bijection type has changed and now they should use Bijection{K,V,F,Finv} for type-stability on their fields.

@mofeing mofeing deleted the merge-bijectivedicts branch May 26, 2025 19:52
@scheinerman
Copy link
Collaborator

We should update the README with a "breaking notice" to tell users that the Bijection type has changed and now they should use Bijection{K,V,F,Finv} for type-stability on their fields.

OK. I can work on that. I'm unclear about Bijection{K,V,F,Finv}. What are these?

  • K is type of the keys
  • V is type of the values
  • F is either Dict or IdDict
  • Finv is either Dict or IdDict

Do I have that right? But when I try to construct a Bijection I get errors:

julia> b = Bijection{Int,Int,IdDict,IdDict}()
ERROR: MethodError: no method matching Bijection{Int64, Int64, IdDict, IdDict}(::IdDict{Any, Any}, ::IdDict{Any, Any})
The type `Bijection{Int64, Int64, IdDict, IdDict}` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  Bijection{K, V, F, Finv}(::Vector) where {K, V, F, Finv}
   @ Bijections ~/.julia/dev/Bijections/src/Bijections.jl:82
  Bijection{K, V, F, Finv}(::Pair...) where {K, V, F, Finv}
   @ Bijections ~/.julia/dev/Bijections/src/Bijections.jl:78

Stacktrace:
 [1] Bijection{Int64, Int64, IdDict, IdDict}()
   @ Bijections ~/.julia/dev/Bijections/src/Bijections.jl:79
 [2] top-level scope
   @ REPL[7]:1

@goerz
Copy link
Collaborator

goerz commented May 26, 2025

I've tried to be as less disruptive as posible

I just started using Bijections in JuliaDocs/DocumenterCitations.jl#95. Are you planning to release this as a breaking release (v0.2)? I might want to double-check that the way I'm using it in DocumenterCitations is still going to work… It's a package that needs to be pretty stable, so I'd want to make sure I'm compatible with a wide range of versions of any dependencies.

@mofeing
Copy link
Collaborator Author

mofeing commented May 26, 2025

In reality, this is more verbose:

  • K is the type of the keys ✅
  • V is the type of the values ✅
  • F is the type of the "forward" mapping; e.g. Dict{K,V}, IdDict{K,V} or any AbstractDict{K,V}
  • Finv is the type of the "inverse" mapping; e.g. Dict{V,K}, IdDict{V,K} or any other AbstractDict{V,K}

Do I have that right? But when I try to construct a Bijection I get errors:

you need to specify the full type of F and Finv

julia> b = Bijection{Int, Int, IdDict{Int,Int}, IdDict{Int,Int}}()
Bijection{Int64, Int64, IdDict{Int64, Int64}, IdDict{Int64, Int64}}()

This is just in the case that you want to customize F and Finv. By default, F and Finv will be specified as Dict{K,V} and Dict{V,K} on the constructors if not specified.

julia> b = Bijection{Int, Int}()
Bijection{Int64, Int64, Dict{Int64, Int64}, Dict{Int64, Int64}}()

@scheinerman
Copy link
Collaborator

I've tried to be as less disruptive as posible

I just started using Bijections in JuliaDocs/DocumenterCitations.jl#95. Are you planning to release this as a breaking release (v0.2)? I might want to double-check that the way I'm using it in DocumenterCitations is still going to work… It's a package that needs to be pretty stable, so I'd want to make sure I'm compatible with a wide range of versions of any dependencies.

Yes! I want to update the documentation (at least the README) before releasing 0.2.0.

@scheinerman
Copy link
Collaborator

This is just in the case that you want to customize F and Finv. By default, F and Finv will be specified as Dict{K,V} and Dict{V,K} on the constructors if not specified.

julia> b = Bijection{Int, Int}()
Bijection{Int64, Int64, Dict{Int64, Int64}, Dict{Int64, Int64}}()

Got it! Gracias.

@mofeing
Copy link
Collaborator Author

mofeing commented May 26, 2025

I just started using Bijections in JuliaDocs/DocumenterCitations.jl#95. Are you planning to release this as a breaking release (v0.2)? I might want to double-check that the way I'm using it in DocumenterCitations is still going to work… It's a package that needs to be pretty stable, so I'd want to make sure I'm compatible with a wide range of versions of any dependencies.

Yes, this is a breaking release but behavior should be the same. The only breaking change is that Bijection now has 2 type parameters more: F and Finv (and 2 fields less).

The only thing you should update is when you have a field that is a Bijection. So, for example, you must change it here: https://github.com/JuliaDocs/DocumenterCitations.jl/blob/52aa72bf4e2a045a2426cf0791fa2bb14ec9f0c6/src/DocumenterCitations.jl#L82

from

anchor_keys::Bijections.Bijection{String,String}

to

anchor_keys::Bijections.Bijection{String,String,Dict{String,String},Dict{String,String}}

Note that you shouldn't need to change anything else. For example, in https://github.com/JuliaDocs/DocumenterCitations.jl/blob/52aa72bf4e2a045a2426cf0791fa2bb14ec9f0c6/src/DocumenterCitations.jl#L129, where you instantiate a Bijection

anchor_keys = Bijections.Bijection{String,String}()

There's no problem because this constructor will default F and Finv type parameters to Dicts.

If it feels too verbose, we can add a macro like @Bijection{K,V} that directly returns Bijection{K,V,Dict{K,V},Dict{V,K}} as a way to lower the barrier.

@scheinerman
Copy link
Collaborator

@mofeing @goerz : The other breaking change is that domain [also image] now returns an iterator and not a Set.

@mofeing
Copy link
Collaborator Author

mofeing commented May 26, 2025

@scheinerman right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants