Skip to content

ExactComputer Refactor Types and Making n_players optional.#450

Merged
mmschlk merged 7 commits intomainfrom
optional-n-players
Oct 21, 2025
Merged

ExactComputer Refactor Types and Making n_players optional.#450
mmschlk merged 7 commits intomainfrom
optional-n-players

Conversation

@mmschlk
Copy link
Copy Markdown
Owner

@mmschlk mmschlk commented Oct 15, 2025

Motivation and Context

This PR closes #388 and improves the ExactComputer's code quality by making the index parameters in method signatures more tightly bound by introducing Literals (e.g. replacing index: str with index: Literal["SV", "BV"]). This reduced in a couple of tidy up operations (introducing match statements for reducing the scope of the variables and pyright compliance.

❓ Remaining Questions:

In ExactComputer.__call__ we compute the Moebius (computed with ExactComputer.moebius_transform) and Co-Moebius (computed with ExactComputer.shapley_base_interaction) while both can be computed via ExactComputer.shapley_base_interaction. Should we harmonize this (see TODOs)? If so should we remove the ExactComputer.moebius_transform property? As I see it now, Moebius is always getting computed with all orders but Co-Moebius not (likewise to all other indices available in ExactComputer.shapley_base_interaction.


Public API Changes

  • No Public API changes
  • Yes, Public API changes (Details below)

How Has This Been Tested?

Added new tests for the cases where the indices may be misspecified and adapted some tests to adhere to the new method signatures.


Checklist

  • The changes have been tested locally.
  • Documentation has been updated (if the public API or usage changes).
  • An entry has been added to CHANGELOG.md (if relevant for users).
  • The code follows the project's style guidelines.
  • I have considered the impact of these changes on the public API.

@mmschlk mmschlk changed the title makes n_players optional ExactComputer Refactor Types and Making n_players optional. Oct 16, 2025
@mmschlk mmschlk requested a review from FFmgll October 16, 2025 15:21
@mmschlk mmschlk self-assigned this Oct 16, 2025
@mmschlk mmschlk added the game theory 🎲📕 Game theory related issues label Oct 16, 2025
@mmschlk mmschlk added this to the 1.4.0 milestone Oct 16, 2025
@mmschlk mmschlk moved this to 👀 In review in shapiq development Oct 16, 2025
Copy link
Copy Markdown
Collaborator

@FFmgll FFmgll left a comment

Choose a reason for hiding this comment

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

computation of moebius transform through moebius_transform() is faster than using shapley_base_interaction(). The latter uses a loop over powerset(grand_coalition) and within loops over powerset(grand_coalition, max_size=k), where k=n for Moebius whereas moebius_transform() requires the inner loop only over powerset(subset from outer loop). While shapley_base_interaction is convenient to compute different values, I assume that other computations could also be quicker with other loops, e.g. Co-Moebius.

The base_interaction() function also allows to only compute up to order k, maybe we could add this to moebius_transform(). In general, I would support having separate functions for each index that are optimized for each index.

Copy link
Copy Markdown
Collaborator

@FFmgll FFmgll left a comment

Choose a reason for hiding this comment

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

rest lgtm

@mmschlk mmschlk merged commit ac56146 into main Oct 21, 2025
10 checks passed
@mmschlk mmschlk deleted the optional-n-players branch October 21, 2025 14:53
@github-project-automation github-project-automation Bot moved this from 👀 In review to ✅ Done in shapiq development Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

game theory 🎲📕 Game theory related issues

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Make n_players Optional in ExactComputer Init

2 participants