-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
There was a problem hiding this 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 😄
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()
Class SelfInfluenceFromLocal(BaseGlobalRankingFromLocal)
Class GlobalRankingFromLocal(BaseGlobalRankingFromLocal)
Class MySpecificGlobalRankingFromLocal(BaseGlobalRankingFromLocal)
def global_ranking(explanations, aggregate_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? |
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: SelfInfluenceGlobalizer: 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!! |
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()
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! |
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. |
Oh, my brain filled the gaps as "From Local Explainer". You meant "FromLocalRanking".
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.
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
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: If the answers are negative, I say we expect the user to pass already initialized Globalizer objects. |
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 😄
Optional[str, BaseGlobalRankingFromLocal] |
Hello Dilya, This is a smaller version. I did a quick implementation for:
Tests are not yet written. I will update this comment on Monday. |
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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
# 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) |
There was a problem hiding this comment.
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 😄
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.