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

minor: Causal fidelity metrics: Harmonization between timeseries and image - detailed evaluate #70

Merged
merged 7 commits into from
Dec 10, 2021

Conversation

AntoninPoche
Copy link
Collaborator

@AntoninPoche AntoninPoche commented Nov 23, 2021

minor: Causal fidelity metrics: Harmonization between timeseries and image - detailed evaluate

This pull request is divided in 3 contributions :

  • Harmonization between:
    • CausalFidelity and CausalFidelityTS edcb5c4
    • Adapt testing to harmonization 03d3849
  • Detailed evaluate for:
  • Remarks in the docs for some metrics:

and a correction for the new version of pylint to work: 22aebd4 (we should use f-string).

Harmonization

The previously developed causal fidelity metrics for timeseries had several differences in their behavior compared to the initial Insertion and Deletion, thus an harmonization was necessary.

Detailed evaluate

Furthermore, through discussion, the necessity of accessing all values computed for insertion and deletion was necessary. The idea thus was to add a method (detailed_evaluate) with returns a dictionnary of such values. This method will then be called by the initial evaluate method that will return the auc.

The output of the detailed evaluate method can be used to draw score evolution curves, where the score depend on the number of features perturbed.

Interpretation and remarks in the documentation

There are remarks about:

  • The score interpretation
  • How max_percentage_perturbed can influence the score
  • The fact that evaluating Insertion and Deletion on the absolute value of explanations may not provide relevant scores.

@fel-thomas
Copy link
Member

You're trying to re-commit commits from last month, can you clean your PR with just the new commits pls ? ;)

@AntoninPoche AntoninPoche force-pushed the antonin/timeseries branch 2 times, most recently from 3b19788 to ea31c39 Compare November 26, 2021 10:49
docs/api/deletion.md Outdated Show resolved Hide resolved
docs/api/insertion.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@lucashervier lucashervier left a comment

Choose a reason for hiding this comment

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

Most of my remarks question the relevance of some change to get more complete metrics. The idea is good but there is a few change to make I believe or to discuss at least. Otherwise, well done 👍 🎉

Copy link
Member

@fel-thomas fel-thomas left a comment

Choose a reason for hiding this comment

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

Cool PR, LGTM ! ;)

docs/api/deletion.md Show resolved Hide resolved
docs/api/deletion_ts.md Show resolved Hide resolved
xplique/metrics/fidelity.py Show resolved Hide resolved
Copy link
Collaborator

@lucashervier lucashervier left a comment

Choose a reason for hiding this comment

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

Thank you for the modification!

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.

None yet

3 participants