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

Fix growing spheres #396

Closed
wants to merge 21 commits into from
Closed

Fix growing spheres #396

wants to merge 21 commits into from

Conversation

kmariuszk
Copy link
Contributor

No description provided.

Comment on lines 25 to 34
function update!(ce::CounterfactualExplanations)
#TODO implement ce.search updating

if (ce.generator == :shrink)
growing_spheres_shrink!(ce)
elseif (ce.generator == :expand)
growing_spheres_expand!(ce)
else (ce.generator == :feature_selection)
feature_selection!(ce)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pat-alt @VincentPikand @RaunoArike do you guys have any better idea on how to implement update!() for growing spheres?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about dispatching on generate_perturbations but I see some problems with the compatibility of the current update method and the one we need for growing spheres. Namely, in a single iteration of growing spheres we create multiple counterfactual candidates but the current implementation of update supports generators creating only one counterfactual at a time.

Also, even if we find a solution to that problem, the original problem of switching between different 'phases' of growing spheres is still there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kmariuszk, sorry I'm only now getting to this, been a very hectic week. Could a set up like in DiCE be applicable where multiple counterfactuals are produced at once?

@kmariuszk
Copy link
Contributor Author

kmariuszk commented Jan 18, 2024

@pat-alt @VincentPikand @RaunoArike would really appreciate if you could give this PR a look and tell me what you think about the direction of changes within growing spheres. I'm very skeptical about adding this new fields to the generator itself to keep the state of it but couldn't find any better solution yet. Thanks!

kmariuszk and others added 2 commits January 26, 2024 18:57
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@izagorac
Copy link

Hi @kmariuszk, what is the current status of this PR? Could I possibly help you with anything? I'm currently doing my thesis with @pat-alt and I want to use the GrowingSpheresGenerator for my research

@kmariuszk
Copy link
Contributor Author

Hi @izagorac, I'll get back to you in a couple of hours

@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.98%. Comparing base (1504e0b) to head (0ff567e).
Report is 481 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #396       +/-   ##
===========================================
- Coverage   77.57%   25.98%   -51.59%     
===========================================
  Files          74       73        -1     
  Lines        1565     1474       -91     
===========================================
- Hits         1214      383      -831     
- Misses        351     1091      +740     

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

kmariuszk and others added 2 commits February 21, 2024 16:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
@JuliaTrustworthyAI JuliaTrustworthyAI deleted a comment from github-actions bot Feb 21, 2024
ce.search[:iteration_count] += 1 # update iteration counter
ce.search[:path] = [ce.search[:path]..., ce.s′]
return terminated(ce)
ce.search[:iteration_count] += 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
ce.search[:iteration_count] += 1
return ce.search[:iteration_count] += 1

Comment on lines 33 to 36
counterfactual = find_counterfactual(
model, target, counterfactual_data, counterfactual_candidates
ce,
counterfactual_candidates
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
counterfactual = find_counterfactual(
model, target, counterfactual_data, counterfactual_candidates
ce,
counterfactual_candidates
)
counterfactual = find_counterfactual(ce, counterfactual_candidates)

@kmariuszk
Copy link
Contributor Author

Hey @izagorac, @pat-alt, I cleaned up the PR and here's a quick summary:

✅ Growing spheres generation has been updated, so now it is actually using the proper convergence logic (i.e., it actually uses the decision_threshold provided by the user.
❌ Growing spheres is still not properly dispatched on generate_perturbations() (currently, it is 'dispatched' on update()). My main concern which blocked me from implementing that is addressed in the next point.
❌ While generating counterfactuals using growing spheres the path cannot be easily updated using current logic. Currently, the update happens in the following line:

ce.search[:path] = [ce.search[:path]..., ce.s′]
the problematic part about that is that GrowingSpheresGenerator is generating multiple points at the same time. I noticed @pat-alt suggested to use similar methodology as in DiCE but unfortunately, I didn't look into it. There might be a possible fix for that.
❔ I'm not sure if currently GrowingSpheresGenerator is capable of generating multiple counterfactuals at once. To be fair, I'm not even sure if the growing spheres algorithm is ever able to generate more counterfactuals than just one.

Currently, I don't have anymore time to look into the mentioned issues. I should have some time I could spend on it on Saturday, @pat-alt do you have time then? We could have a pair-programming session which could speed-up the whole process. Otherwise, @izagorac you can try to fix the mentioned issues by yourself.

@izagorac
Copy link

Thank you for the elaborate summary @kmariuszk !

First of all, you don't have to worry about fixing this issue ASAP because I still have loads to work on for my thesis and if these issues take a while to fix that is no problem.
Second, I see you mentioned the amount of counterfactuals that the generator can generate at once. Do you refer to the number of counterfactuals that it will generate per factual, which can be specified with the num_counterfactuals parameter in the generate_counterfactual function, or the amount of factuals that counterfactuals can be generated for (hopefully this sentence still makes sense). If you mean the first one, then I don't see any issue for my work. I will namely only be generating one counterfactual per factual.

Anyways, thanks for your time and hopefully we can solve these issues together!

@pat-alt
Copy link
Member

pat-alt commented Feb 21, 2024

Thanks @kmariuszk for looking into this. I'm at AAAI in Canada right now and we have a pretty packed schedule, even on the weekend. I could probably skip a session on Saturday, but then I'm still in a very different time zone 😅 perhaps easier when I'm back in early March.

In the meantime, might it be worth already merging what we have here? I'm puzzled by the errors on 1.7 and 1.8, but that should be fixable. @izagorac would that fix your current issue? If not, could you post the error your getting here?

@izagorac
Copy link

@pat-alt @kmariuszk I'm not sure whether the updates fix my current issues but I can always use the updated generator if it gets merged. Nonetheless, I'll post the exact error I get here:

Screenshot 2024-02-22 at 10 55 47

From looking at the error message, it seems to be a very generic error. However, if I run the exact same code but with the FeatureTweakGenerator, I have no problems at all. Hopefully, this stacktrace gives you some additional information @kmariuszk.

@pat-alt
Copy link
Member

pat-alt commented Sep 23, 2024

@kmariuszk it's been a minute 😅 how are you doing? Do you think you'll ever revisit this? Otherwise I'll probably remove GrowingSpheres from the public API and docs

@kmariuszk
Copy link
Contributor Author

Hey @pat-alt, I'm afraid I won't have time to revisit this, so feel free to remove GrowingSpheres.

@pat-alt pat-alt closed this Dec 19, 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

Successfully merging this pull request may close these issues.

3 participants