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

Simple feature fixes #1419

Merged
merged 16 commits into from May 12, 2022
Merged

Simple feature fixes #1419

merged 16 commits into from May 12, 2022

Conversation

JKL98ISR
Copy link
Member

@JKL98ISR JKL98ISR commented May 11, 2022

closes #1004

docs have now example per class and per all. added min pps to init + test
also fixed not showing negative difference in the condition (if train < test)

@JKL98ISR JKL98ISR requested a review from a team as a code owner May 11, 2022 07:53
@JKL98ISR JKL98ISR self-assigned this May 11, 2022
@JKL98ISR JKL98ISR requested review from a team, ItayGabbay and shir22 as code owners May 11, 2022 07:53
@@ -180,7 +174,7 @@ def mnist_batch_to_images_with_bias(batch):

#%%

check = SimpleFeatureContribution()
check = SimpleFeatureContribution(per_class=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

But... you just reverted the doc to use the per_class=False. But we do want to show the per_class=True feature. We actually want to show both.

Copy link
Member Author

Choose a reason for hiding this comment

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

it has both

Copy link
Member Author

Choose a reason for hiding this comment

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

mnist is per class

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

test_df: pd.DataFrame,
test_label_name: Optional[Hashable], ppscore_params: dict,
n_show_top: int,
min_pps_to_show: float = 0.05,
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't add min_pps_to_show to the check's parameters...
To this one or to single feature

Copy link
Member Author

Choose a reason for hiding this comment

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

i added to simple feature, should i add to single too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so. Also missed that it was added to the check, sorry

Copy link
Member Author

Choose a reason for hiding this comment

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

added

@JKL98ISR
Copy link
Member Author

coco after bias
Screen Shot 2022-05-11 at 16 21 22
mnist after bias
Screen Shot 2022-05-11 at 16 21 36
coco no bias:
Screen Shot 2022-05-11 at 16 22 53

@nirhutnik
Copy link
Contributor

  1. MNIST after bias - there's rarely any difference... Did you insert bias both into train and test? It should be inserted differently, if at all (can only insert to train, but good to insert a bit into test)

  2. COCO - because there is already a bit of bias, I would insert only into train. Again, the differences here as well are not significant. We want to have an example with a real difference.

@JKL98ISR JKL98ISR enabled auto-merge (squash) May 12, 2022 09:31
@JKL98ISR JKL98ISR disabled auto-merge May 12, 2022 09:37
@JKL98ISR JKL98ISR enabled auto-merge (squash) May 12, 2022 09:41
@JKL98ISR JKL98ISR changed the title Single feature fixes Simple feature fixes May 12, 2022
@JKL98ISR JKL98ISR merged commit 0b05fca into main May 12, 2022
@delete-merged-branch delete-merged-branch bot deleted the sngle_feature_min branch May 12, 2022 11:00
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.

Add "per class" to SimpleFeatureContribution
3 participants