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

Simplification of the core struct and related methods #360

Closed
wants to merge 138 commits into from

Conversation

RaunoArike
Copy link
Contributor

@RaunoArike RaunoArike commented Nov 23, 2023

To simplify the core struct and related methods, I propose the following changes:

  • Create new structs for all different convergence criteria, and dispatch based on the types of these structs when calling the converged() method. This removes a ton of kwargs from the main struct and makes the code clearer in general. For the more casual users, nothing changes after this refactor: they can still use the :converge_when keyword argument to specify a convergence criterion with the default arguments. The more advanced user will need to be a bit more involved than before to define custom convergence criteria: they will have to define one of the new convergence structs on their own and pass that to the generate_counterfactual() method. However, the improved clarity of the code seems to justify the added hassle for those users.
  • Remove the converged() method that had been created specifically for Growing Spheres. There's no reason Growing Spheres shouldn't fit into the pre-existing convergence framework.
  • Remove the :early_stopping convergence type, which appears to be functionally the same as the :max_iter convergence type.
  • Move the generative_model_params kwarg out from the CounterfactualExplanation struct and define it as a kwarg of the GradientBasedGenerator struct instead, since it's only related to the generators.
  • Remove the learning_rate keyword argument, as it's not used anywhere in the package.
  • Move the files that were previously in the generate_counterfactual folder to the counterfactuals folder. Since the primary functionality of the methods in that folder is instantiating CounterfactualExplanation structs, which are defined in the counterfactuals folder, it makes more sense for these methods to be defined inside the same folder with the struct.
  • This is a slightly unrelated change, but the Generator struct was not used anywhere in the package, so I also removed that, as well as a few other pieces of redundant code.

Also, sorry for the enormous size of the PR. I'll keep the next ones shorter.

@RaunoArike RaunoArike self-assigned this Nov 23, 2023
@RaunoArike RaunoArike marked this pull request as draft November 23, 2023 22:33
src/generators/gradient_based/base.jl Outdated Show resolved Hide resolved
src/generators/gradient_based/probe.jl Outdated Show resolved Hide resolved
src/convergence/decision_threshold.jl Outdated Show resolved Hide resolved
src/convergence/invalidation_rate.jl Outdated Show resolved Hide resolved
RaunoArike and others added 4 commits November 28, 2023 19:19
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>
This reverts commit 903033e.
This reverts commit a8c8139.
This reverts commit 56d7279.
This reverts commit bd0f42c.
This reverts commit c61765d.
This reverts commit 16844b7.
This reverts commit 612fb94.
This reverts commit c1ef286.
This reverts commit e86c613.
This reverts commit fdd4759.
This reverts commit d95841c.
This reverts commit 338167d.
This reverts commit a7a1211.
This reverts commit 96968ae.
This reverts commit ed30593.
This reverts commit 1abc592.
This reverts commit 80f23d2.
This reverts commit addbbec.
@@ -40,4 +40,4 @@ function Flux.Losses.mse(ce::AbstractCounterfactualExplanation; kwargs...)
kwargs...,
)
return loss
end
end
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
end
end

This reverts commit 0067cf5.
@pat-alt
Copy link
Member

pat-alt commented Dec 8, 2023

Hi @VincentPikand @RaunoArike @kmariuszk, I tried to fix this thinking it would be quick work:

For some reason I can't reply directly to this comment:

It is differentiated, see lines 39

That seems wrong to me: looking at the paper, it seems we first need taking gradients to compute the approximation of IR (which is what's done here). But then we still need to differentiate the counterfactual search with respect to that estimated quantity.

I don't remember enough to answer this currently and as such we can make this a separate issue as it is kind of unrelated to the changes of this PR.

But @VincentPikand was right, should have parked this for a new issue and PR (sorry!). I've reverted everything to 0f2560e here now and opened #371 which contains all the modifications that I've made, so let's take it from there. There's another inconsistency I've noted and will detail in that draft PR.

There are a few commits here such as b77e8a8 and e6fcaa0 that I've reverted that we might want to re-revert (sorry again!). Maybe you can all have another quick look to double-check?

After that, happy to merge this one

@VincentPikand
Copy link
Contributor

@pat-alt (and my future employers who are looking into my open source contributions), pardon my french, but this PR is fucked up beyond repair. We've agreed to split this up as much as possible into digestible bites instead of 23-1 fasting megatron meals.

@pat-alt
Copy link
Member

pat-alt commented Dec 8, 2023

Lmao yeh let's do that

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.

4 participants