Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Discrepancies Between test_all() and test_ambiguities() #258

Closed
kmariuszk opened this issue Jan 11, 2024 · 1 comment
Closed

Bug: Discrepancies Between test_all() and test_ambiguities() #258

kmariuszk opened this issue Jan 11, 2024 · 1 comment

Comments

@kmariuszk
Copy link

I have been trying to add the Aqua package to CounterfactualExplanations.jl repository. I have run into the following problem:

Initially, I started with the regular Aqua.test_all(CounterfactualExplanations but it threw almost 90 ambiguities, and basically none of them was from the package itself. For example:

Ambiguity #1
<(a::Integer, b::SentinelArrays.ChainedVectorIndex) @ SentinelArrays ~/.julia/packages/SentinelArrays/1kRo4/src/chainedvector.jl:208
<(x::BigInt, i::Integer) @ Base.GMP gmp.jl:711

is from the SentinelArrays package.

I noticed there's no such problem when running Aqua.test_ambiguities([CounterfactualExplanations]; recursive=false, broken=false). Then, there are only two relevant ambiguities detected.

After quick digging in the codebase, I noticed the test set for ambiguity in the test_all() method includes some additional modules in the function call, i.e.:

@testset "Method ambiguity" begin
        if ambiguities !== false
            test_ambiguities([testtarget, Base, Core]; askwargs(ambiguities)...)
        end
    end

I suppose that's what causes the discrepancies.

I would be very thankful if someone who knows the package better could pivot me in the right direction.

@lgoettgens
Copy link
Collaborator

Duplicate of #77

The issue with Aqua.test_ambiguities([CounterfactualExplanations]; recursive=false, broken=false) is that it only finds ambiguities between functions of your package. Ambiguities that you introduce to a Base function may be skipped. Furthermore, there are some problems with ambiguities in constructors.

Once I find some time for #180, you would get better possibilities to use the ambiguity test. For the time being, I recommend to either use your workaround (and miss some ambiguities) or skip the test altogether.

@lgoettgens lgoettgens closed this as not planned Won't fix, can't repro, duplicate, stale Jan 11, 2024
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

No branches or pull requests

2 participants