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

WiP: Globalization from local explainer #24

Merged
merged 19 commits into from
Jun 14, 2024
Merged

WiP: Globalization from local explainer #24

merged 19 commits into from
Jun 14, 2024

Conversation

gumityolcu
Copy link
Collaborator

@gumityolcu gumityolcu commented Jun 4, 2024

This is work in progress but I would like some feedback about the main mentality of my implementation which I explain below:

The Globalization object is something that either takes explanations or an explainer and generates a global ranking of the training data. Two obvious options is to sum up the absolute values of local explanations, or look at the self-influence of the datapoints. However, we would like to allow different strategies, also custom strategies by the user so we are dedicating a class for this. In general, we could talk about Globalization that uses local explanations, or by using a function that computes some scalar for each training datapoint.

However, the two approaches are very different. This is why I made 2 subclasses, one that can be used with .update(), and another one that takes a attributor_fn function. The user can select one of these classes to subclass for their implementation. Inside our metrics, we can just check which one of these two the object (passed by the user) belongs to. So while it may seem like subclass-inflation, I think this really makes sense. Because I feel like the two versions of Globalization should have seperate functions and seperate ways to use them. These two classes provide an easy way to allow the user to subclass, while having limited complexity inside the metrics dedicated to these two main subclasses.

Why do I think the two approaches differ a lot:
1- There is no sensible notion of .update for the second option. But the first one only needs a .update that easily parallels the metric.update methods
2- The second one takes a function and kwargs, and it seems like making two subclasses and checking isinstance for them is wiser than validating the different parameters at initialization
3- It makes it much easier to subclass by the user. For one we just say "use .update to implement your strategy for aggregating local explanations", and for the second we say "provide us with an attributor_fn that computes some score for each training data"

I am not using the term explain_fn, instead I use attributor_fn to distinguish due to the following reasons:
1- It does not HAVE TO be an explanation function. I mean, the user could give a function that computes something else.
2- If we want self-influence, the explanation functions needs to attribute only the training datapoint with the given index, not the whole training dataset. So it's signature is different from the explain_fn that is used elsewhere in the library, for example in the model randomization test.

I would really appreciate your comments on these issues.

@gumityolcu gumityolcu requested a review from dilyabareeva June 4, 2024 15:55
Copy link
Owner

@dilyabareeva dilyabareeva left a comment

Choose a reason for hiding this comment

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

Hi Galip! Thank you for this, looks very good!

I think the main thing about splitting the "Globalization" into two subclasses, is to make sure that they follow a consistent interface, reflected in their base class. We want to make sure that metrics implementations are agnostic to a "Globalization" class. I am not sure that it is possible at this stage, but let's strive towards it.

Just as a suggestion, we could unite all of those under an interface similar to base Metric, and also create functional version of these. It seems that the core difference is that self-influence is more like a function, while the sum aggregator can be implemented like a memory-full metric. While memory-full self-influence, or functional sum aggregator are not very useful, we could do this for the sake of consistency. This idea is not super well-thought out, just throwing it our there though 😄

src/utils/globalization/base.py Outdated Show resolved Hide resolved
src/utils/globalization/base.py Outdated Show resolved Hide resolved
src/utils/globalization/base.py Outdated Show resolved Hide resolved
src/utils/globalization/from_explanations.py Outdated Show resolved Hide resolved
src/utils/globalization/from_explainer.py Outdated Show resolved Hide resolved
src/utils/globalization/from_explanations.py Outdated Show resolved Hide resolved
src/utils/globalization/from_explainer.py Outdated Show resolved Hide resolved
@dilyabareeva
Copy link
Owner

dilyabareeva commented Jun 7, 2024

Hi @gumityolcu, I don't remember if we agreed on this specifically, but I think we have reached a consensus on the following model:

Class BaseGlobalRankingFromLocal()

  • method: update(explanations, *args, **kwargs)
  • method: compute() -> torch.Tensor
  • reset, load_state_dic, state_dict

Class SelfInfluenceFromLocal(BaseGlobalRankingFromLocal)

  • method: update(explanations, *args, **kwargs)
  • method: compute() -> torch.Tensor
  • reset, load_state_dic, state_dict

Class GlobalRankingFromLocal(BaseGlobalRankingFromLocal)

  • init(aggregate_fn: Callable, *args, **kwargs)
  • method: update(explanations, *args, **kwargs)
  • method: compute() -> torch.Tensor
  • reset, load_state_dic, state_dict

Class MySpecificGlobalRankingFromLocal(BaseGlobalRankingFromLocal)

  • init(*args, **kwargs)
  • method: update(explanations, *args, **kwargs)
  • method: compute() -> torch.Tensor
  • reset, load_state_dic, state_dict

def global_ranking(explanations, aggregate_fn, train_dataset, *args, **kwargs) -> torch.Tensor
def my_specific_global_ranking(explanations, train_dataset, *args, **kwargs) -> torch.Tensor
def self_influence(explain_fn, train_dataset, *args, **kwargs) -> torch.Tensor

Caveat: in most evaluators we will use the functional self-influence, because it is more efficient, and we only calculate diagonal elements.

Are we on the same page?

@gumityolcu
Copy link
Collaborator Author

I am just confused that all classes have a update method. Why does SelfInfluence have it? In general, I feel that .update should not be present in the base class. The shared functionality between the two is 'compute_ranking' and 'return this particular order instead of whatever you would normally do'. So I propose we get rid of the MySpecificGlobalRankingFromLocal. We add the 'force ranking' functionality to the base class and don't have a update method in the base class.

to make things clear: this is how Globalizers will be used, right?

AggregatingGlobalizer:
User aggregates explanations with .update and passes the globalizer to the DownStreamTaskEvaluator that uses an underlying global metric to evaluate the ouput of the Globalizer. They could also compute the ground truth and use a GlobalMetric such as global_metric(globalizer.compute_ranking(), ground_truth)

SelfInfluenceGlobalizer:
User creates the object and populates the self influences, then passes this object to ta DownStreamTaskEvaluator. They can also use it with a global metric.

I don't see the benefit of functional versions, but I also see no disadvantages to implement them. So let's do it!

Does this sound like it's parallel to what you are thinking? Because I couldn't be sure just by reading the class descriptions.

Thanks!!

@dilyabareeva
Copy link
Owner

dilyabareeva commented Jun 7, 2024

Thanks, @gumityolcu! I was hoping that this would be clearer from my class descriptions, but I will try to get my point across again 😄

All the classes in my suggestion above only aggregate from already existing explanations, hence ...FromLocal. Why? Because if someone already has local explanations on the disk/in memory, they don't want to recalculate them, even if it is just diagonal elements for SelfInfluence.

The self_influence function, however, generates diagonal entries from scratch. I didn't add a matching class, but we could (not to be confused with SelfInfluenceFromLocal):

Class SelfInfluence()

  • method: evaluate(train_dataset, explain_fn, *args, **kwargs)

Do we need some base class for SelfInfluence? Maybe, but I can't think of any similar global rankers.

Those are stand-alone explainers, evaluators are a totally independent logic. Inside of (downstream-task-)evaluators though, I would only use functional versions that take the whole set of explanations and run the loop inside. That is why I think for now self_influence functional is more relevant for us. But maybe the user would want to run SelfInfluence (not ...FromLocal) on their own, we wouldn't want to stop them from it.

Just clarifying my earlier message. I think it all fits into nice logical boxes and with overall design structure of the library this way! Would be happy about your feedback!

@dilyabareeva
Copy link
Owner

dilyabareeva commented Jun 7, 2024

Also, to clarify, in this example, SelfInfluence() and BaseGlobalRankingFromLocal() do not share any ancestor classes. They are totally split. I don't see the point of unifying them just for the sake of get_score() method, I think they are too different.

@gumityolcu
Copy link
Collaborator Author

gumityolcu commented Jun 7, 2024

All the classes in my suggestion above only aggregate from already existing explanations, hence ...FromLocal.

Oh, my brain filled the gaps as "From Local Explainer". You meant "FromLocalRanking".

The self_influence function, however, generates diagonal entries from scratch. I didn't add a matching class, but we could (not to be confused with SelfInfluenceFromLocal):

What exactly does SelfInfluenceFromLocal do? As far as I understand, it takes precomputed full training data explanations ( a tensor with shape (training_data_len, training_data_len) ) and just collects the diagonal elements, right? I think it's very easy for the user to collect the diagonal elements and only pass those to the globalizer to say "Use this ranking at all times, don't expect a method call". This way, the user could also prefer any other ranking that they precomputed according to any other strategy.

Do we need some base class for SelfInfluence? Maybe, but I can't think of any similar global rankers.

Let's stick with the simpler option, no class version for now. It is not a big hassle to add the class version in the future imo

Those are stand-alone explainers, evaluators are a totally independent logic. Inside of (downstream-task-)evaluators though, I would only use functional versions that take the whole set of explanations and run the loop inside.

This is where I have a different view. It is definitely easier to use the functional versions but the first doubt that i have is: how does the user select which kind of Globalizer they want to use inside an evaluator? I was thinking: they pass objects so they have full control over what kind of globalization is done.

Another thing is: In the evaluators with AggregatibgGlobalizers, we will generate explanations batch by batch. Do we want to save explanations to files to create explanations objects and call the functional versions of AggregatingGlobalizer?

So in summary

three questions, in the case we use functional versions inside evaluators:
1- How will the user determine which globalizer is used
2- Does functional version allow for subclassing and customization by the user
3- Wouldn't this add needless disk write/read operations just to avoid using the class versions?

If the answers are negative, I say we expect the user to pass already initialized Globalizer objects.

@dilyabareeva
Copy link
Owner

What exactly does SelfInfluenceFromLocal do? As far as I understand, it takes precomputed full training data explanations ( a tensor with shape (training_data_len, training_data_len) ) and just collects the diagonal elements, right? I think it's very easy for the user to collect the diagonal elements and only pass those to the globalizer to say "Use this ranking at all times, don't expect a method call". This way, the user could also prefer any other ranking that they precomputed according to any other strategy.

Yes, it takes the diagonal elements. I actually have to disagree with you on this one. An Aggregator/Globalizer class has a functionality calculate global rank, if it only persists the ranking and does not calculate it, it breaks the logic and it is not elegant. A single interface that unites all of the globalizers is more convenient and reduces a user's mental load.

Sometimes engineering software is not about what is ✨easy to do✨ for the user but what does not take a user out of their flow, it's about what ✨sparks joy✨. If I play around with a library, I have my explanations and I'm trying different ways to aggregate them, and then I find that for Self-Influence aggregation I have to write some code? I can't just replace SumAggregator with SelfInfluence? Ugh! What a buzz kill 😄

So in summary

three questions, in the case we use functional versions inside evaluators: 1- How will the user determine which globalizer is used 2- Does functional version allow for subclassing and customization by the user 3- Wouldn't this add needless disk write/read operations just to avoid using the class versions?

Optional[str, BaseGlobalRankingFromLocal]

@gumityolcu
Copy link
Collaborator Author

Hello Dilya,

This is a smaller version. I did a quick implementation for:

  1. Base class ExplanationsAggregator for aggregation interface with update and get_global_ranking methods in utils/aggregators.py
  2. SumAggregator which just sums up the explanations and AbsSumAggregator which sums up the absolute values. In the same file.
  3. a function get_self_influence_ranking in utils/common.py as the functional interface for self-influence global ranking
  4. class SelfInfluenceFunction(Protocol) in explainer_wrapper.py specifies the arguments for self_influence_fn

Tests are not yet written. I will update this comment on Monday.

src/utils/aggregators.py Outdated Show resolved Hide resolved
src/utils/common.py Outdated Show resolved Hide resolved
src/utils/explain_wrapper.py Outdated Show resolved Hide resolved
src/utils/common.py Outdated Show resolved Hide resolved
src/utils/aggregators.py Outdated Show resolved Hide resolved
@dilyabareeva
Copy link
Owner

Hi @gumityolcu, thanks a lot for your work! Everything looks good, I think we could merge. I just have a few comments/suggestions, would be happy if you could go through them. Then I think we could merge and move on to metrics then use this functionality.

Copy link
Owner

@dilyabareeva dilyabareeva left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Added some small comments. Let's fix the test and merge! (so that the PRs don't pile up)

train_dataset: torch.utils.data.Dataset,
test_tensor: torch.Tensor,
method: str,
test_target: Optional[torch.Tensor] = None,
Copy link
Owner

Choose a reason for hiding this comment

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

what is the difference between test_target and test_tensor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test_target is which output we are explaining. There is no such notion for similarity of intermediate representations, but in general explainers can be used to explain different outputs.

src/metrics/unnamed/top_k_overlap.py Outdated Show resolved Hide resolved
src/utils/explain_wrapper.py Show resolved Hide resolved
@gumityolcu gumityolcu merged commit f06f535 into main Jun 14, 2024
2 checks passed
# TODO: I don't know why Captum return test activations as a list
if isinstance(test, list):
test = torch.cat(test)
assert torch.all(test == train)
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @gumityolcu, please let's be more careful 😄

@dilyabareeva dilyabareeva deleted the globalization branch July 2, 2024 07:04
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.

2 participants