Skip to content

Conversation

@alexcombessie alexcombessie self-assigned this Jul 8, 2022
@alexcombessie alexcombessie marked this pull request as draft July 8, 2022 21:24
@alexcombessie alexcombessie requested a review from andreybavt July 13, 2022 18:07
@alexcombessie alexcombessie marked this pull request as ready for review July 13, 2022 18:07
@alexcombessie
Copy link
Member Author

@andreybavt To check what it's doing, you can check the demo projects:

1. Zillow price prediction - regression

BEFORE
Screenshot 2022-07-13 at 20 18 26

AFTER
Screenshot 2022-07-13 at 20 15 04

2. German credit scoring - binary classification

BEFORE
Screenshot 2022-07-13 at 20 18 51

AFTER
Screenshot 2022-07-13 at 20 14 10

5. ENRON - multi classification

BEFORE

Screenshot 2022-07-13 at 20 17 59

AFTER

Screenshot 2022-07-13 at 20 15 43

@alexcombessie alexcombessie changed the title Sort simple explanation chart Jul 14, 2022
Copy link
Contributor

@andreybavt andreybavt left a comment

Choose a reason for hiding this comment

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

I left some minor comments, plus there are a few "Code smells" from Sonar. Let me know if you have time to finish this PR, otherwise, I can do it myself @alexcombessie

const explanationSumByFeature: object = {};
topFeatures.forEach((element, index) => {
explanationSumByFeature[element] = explanationSum[index];
});
Copy link
Contributor

Choose a reason for hiding this comment

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

simplification nitpick: explanationSum auxiliary array (and some other aux variables) creation can be avoided if you replace lines 158-170 with something like this:

   // Compute the sum of SHAP explanations by feature
    const explanationSumByFeature: { [name: string]: number } = _.reduce(
        _.values(this.fullExplanations),
        (acc, labelExplanations) => {
          _.forOwn(labelExplanations, (featureName, explainValue) => {
            acc[explainValue] = (acc[explainValue] || 0) + featureName;
          });
          return acc;
        }, {}
    )

here I'm using lodash that you can import with import _ from "lodash";

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but I wonder if this version is not less readable? Also, are you 100% sure this is the same formula as the code I wrote? Just asking, since I spent over 3 hours to craft the formula and make sure it was correct.

Copy link
Contributor

@andreybavt andreybavt Jul 18, 2022

Choose a reason for hiding this comment

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

If I'm not missing anything, your transforming Category->feature->explanation nested dictionary into feature->explanation dictionary by summing up explanations by feature across categories

Copy link
Member Author

Choose a reason for hiding this comment

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

yep exactly, with sorting

@alexcombessie
Copy link
Member Author

I left some minor comments, plus there are a few "Code smells" from Sonar. Let me know if you have time to finish this PR, otherwise, I can do it myself @alexcombessie

Thanks! I took care of the code smells directly in 483a246

For the rest, I won't have time before next week. This afternoon I need to work on the EIC proposal, on which I am already quite late. So yeah if you can finish it, that would be fantastic, thanks!

@andreybavt andreybavt merged commit 8f67002 into main Jul 18, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Hartorn Hartorn deleted the bug/GSK-158-sort-shap-plot branch September 13, 2023 11:29
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants