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

Lars/add MSEv eval criterion #357

Merged
merged 165 commits into from
Dec 11, 2023

Conversation

LHBO
Copy link
Collaborator

@LHBO LHBO commented Aug 24, 2023

Implemented the Mean Squared Error (MSEv evaluation criterion) of the contribution function v(s) as proposed by Frye et al. (2019) and used by Olsen et al. (2022).

This evaluation criterion is computed for all approaches as long as internal$parameters$output_size == 1.

Have also added plot functions and included a section in the vignette.

LHBO added 4 commits August 24, 2023 10:47
…g several approaches the old version only used the first approach. Verified this by adding a print in each prepare_data.approach() function and saw that only the first approach in internal$parameters$approach was used.

Can maybe remove code comments before pull request is accepted. Maybe a better method to get the approach?

Also updated roxygen2 for the function, as it seemed that it was reflecting the old version of shapr(?) due to arguments which are no longer present.

However, one then get a warning when creating the roxygen2 documentation. Discuss some solutions as comments below. Discuss with Martin.
…all approaches as long as `internal$parameters$output_size == 1`.

Need to think about if it is applicable for vector of outputs.
This function is a separate plot function and is not part of the `shapr.plot()` function. It would maybe nice to make it a part of it and using, e.g., `plot_type = "MSEv". However, `make_MSEv_evaluation_criterion_plots()` handles list of explanation objects while `shapr.plot()` is restricted to a single shapr explanation object. Thus, one would need to rewrite the `shapr.plot()` to also handle mulitple objects.
@LHBO
Copy link
Collaborator Author

LHBO commented Aug 24, 2023

The checks fail, and it seems like it is because the vignette does not find the new make_MSEv_evaluation_cirterion_plots() function. I do not understand why that happens. The function is exported. Do I need to add shapr:: before the function call? But that is not needed when we call the explain() function.

LHBO added 8 commits August 24, 2023 12:10
…ion `check_n_batches` threw an error for the vignette with gaussian approach with `n_combinations` = 8 and `n_batches = NULL`, as this function here then set `n_batches` = 10, which was too large. We subtract 1 as `check_n_batches` function specifies that `n_batches` must be strictly less than `n_combinations`.
…to 2^m", but all the test only tested for "larger than". I.e., if the user specified n_combinations = 2^m in the call to shapr::explain, the function would not treat it as exact.
…ach by default.

Futhermore, there was a logical error.
… an internal function.

Could make a test out of the examples too.
…and to make it easier for the user to controll what figures that are made
…ks with bars), but not the col (which works for lines/points).
@LHBO
Copy link
Collaborator Author

LHBO commented Aug 30, 2023

The errors now seem to be related to some test failures. These tests compare with the previous output from the explain() function, but now the output from the explain() function also includes the MSEv evaluation criterion. Note 100% sure. Discuss with Martin if I return the MSEv correct place. See the vignette for usage of the MSEv.

LHBO added 16 commits August 31, 2023 11:34
…t mode when `n_combinations = 2^m`, before the bugfix.
…`n_combinations >= 2^m`. Remove the large comment after discussing that with Martin.
…est checking that we do not get an error when runing the code after the bugfix has been applied.
… the number of approaches and the number of unique approaches.

This is for example useful to check that the provided `n_batches` is a valid value. (see next commits)
…the number of unique approaches. Before the user could, e.g., set `n_batches = 2`, but use 4 approaches and then shapr would use 4 but not update `n_batches` and without giwing a warning to the user.
… of unique approaches that is used. This was not done before and gave inconsistency in what number shapr would reccomend and use when `n_batches` was set to `null` by the user.
…ombined approaches.

Furthermore, added if test, because previous version resulted in not reproducible code, as setting seed to `null` ruins that we set seed in `explain()`.

Just consider this small example:
# Set seed to get same values twice
set.seed(123)
rnorm(1)

# Seting the same seed gives the same value
set.seed(123)
rnorm(1)

# If we also include null then the seed is removed and we do not get the same value
set.seed(123)
set.seed(NULL)
rnorm(1)

# Setining seed to null actually gives a new "random" number each time.
set.seed(123)
set.seed(NULL)
rnorm(1)
@LHBO
Copy link
Collaborator Author

LHBO commented Dec 1, 2023

see some comments

Have rewritten the code based on your feedback.
I have run lintr::lint_dir(), styler:::style_dir(), and devtools::run_examples().
We I run testthat(), then I get a lot of errors which are due to the objects being changed due to e.g. new names.

@martinju
Copy link
Member

martinju commented Dec 8, 2023

I think this looks good now. I have some minor suggestions for name changes:

make_MSEv_eval_crit_plots -> plot_MSEv_eval_crit
And in this function:
level = 0.95 -> CI_level = ifelse(length(explanation_list[[1]]$pred_explain)<20, NULL, 0.95) [name change + don't plot CI if there are fewer than (say) 20 obs]
make_MSEv_comb_and_explicand -> plot_type = "overall" [a character vector specifying which plots to produce, can take "overall", "comb" and "explicand"]

What do you think, @LHBO ?

Copy link
Member

@martinju martinju left a comment

Choose a reason for hiding this comment

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

I fixed a few minor issues. I think this is good for now.
Feel free to merge if you agree.

@LHBO
Copy link
Collaborator Author

LHBO commented Dec 11, 2023

Looked at your final updates and agree with them

@LHBO LHBO merged commit 579724b into NorskRegnesentral:master Dec 11, 2023
7 checks passed
@LHBO LHBO deleted the Lars/add_MSEv_eval_criterion branch December 11, 2023 14:24
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