Re-add composition with ∘ and new compose function

[goerz]

This would be my suggestion to close #30

It partially reverts #39, re-adding the functionality, documentation, and tests, but renaming * to ∘ and implementing it in terms of a new (public) compose function.

I'm not strongly tied to this, so it's fine to close if you prefer to remove composition entirely.

This is missing an entry in the CHANGELOG, but since this is all very much up in the air, I'd add that separately after a merge.

We should also make sure the code snippets in docs/src/operations.md are automatically verified as doctests, but I'll also address that separately in #42

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.65%. Comparing base (ed40d49) to head (c0b42c2).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   85.89%   88.65%   +2.76%     
==========================================
  Files           1        1              
  Lines          78       97      +19     
==========================================
+ Hits           67       86      +19     
  Misses         11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mofeing]

My problem with this is that we are not preserving the dictionary types. Remember that Bijection now has it's dictionary types parameterized so if I do the following:

a = Bijection{U,V,F_a,Finv_a}(...)
b = Bijection{V,W,F_b,Finv_b}(...)

c = compose(a, b)

What should be the type of F_c and Finv_c? This PR assumes is gonna be Dict{U,V} and Dict{V,U}, which is broken when U or V are mutable types and you use a IdDict (e.g. when U or V are Array).

This was trivial before when they weren't parameterized (i.e. they would be Dict) but it's not clear to me now. Changing this behavior in the future might be seen as a breaking change, and since I've searched and seen no users of this feature (could be wrong but did an exhaustive search on the dependent libraries shown in JuliaHub and in GitHub code search), I think it's better to wait.

Also, the functionality introduced here is not critical and it can still be done in a couple of lines:

# we define the dictionary types of `c` how we would like
c = Bijection{U,W,F_c,Finv_c}()

for (u, v) in a
    c[u] = b[v]
end

# or even in one line
c = Bijection{U,W,F_c,Finv_c}([u => b[v] for (u,v) in a])

In summary, I see this as something that can give us more problems than advantages so I would refrain from adding it to the v1.0 release. It can be thought for the v1.1 release if we find a way to circunvent the issue stated above.

PS: It seems like the one-liner above doesn't work with Base.Generator, which we could easily add support for it.

[goerz]

Ah! Now I finally understand the problem! :-)

This is quite tricky indeed. I've been playing around with some ideas, and added a potential way to handle this in the latest commit. Fundamentally, this adds a composed_dict_type function that figures out the type for the composed dict. At least, this will preserve the mapping type both for the default Dict and for anything involving an IdDict (our documented way of handling mutable values). It falls back to the same internal feature called out in #26.

However, to take this further, it's not really clear that there is an obvious correct answer for all possible combinations of AbstractDicts. When combining an OrderedDict and an IdDict, what property should be preserved (my guess would be IdDict, but who's to say). Plus, you have a combinatorial explosion that's quadratic in all possible subtypes of AbstractDicts. It just might not be worth the trouble. I suppose the question of for @scheinerman to weigh in on what the composition feature is really intended for. That is, what was the motivation for adding the feature in 7d3a25c.

The behavior currently implemented is well-defined, but arbitrary.

I'd be okay with keeping the feature out and simply noting in the changelog that composition was removed because the type of the internal forward and inverse mapping is ill-defined. This works as probably the simplest replacement for the feature:

Bijection((x => a[b[x]] for x in keys(b))...)

[goerz]

So, yeah, I think my vote to agree with @mofeing and to drop this. We can keep this PR around for if we ever want to get back to it post-1.0. Preferably after someone opens an issue demonstrating a specific use case, so that we can make sure that whatever implementation we use meets that use case, as opposed to just me speculating what "correct" behavior should be.

[scheinerman]

@goerz @mofeing : I see this PR is open. Do I need to do something?

[goerz]

No. This is just some code we can keep around for some indefinite future. If someone ever comes around with a specific use case for composition, we could look at whether the code in this PR might be a useful starting point.