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

Refactor MSR Banzhaf valuation #605

Merged
merged 14 commits into from
Jul 9, 2024

Conversation

janosg
Copy link
Collaborator

@janosg janosg commented Jun 19, 2024

Description

This PR implements Maximum Sample Re-use (MSR) Banzhaf valuation in the new architecture. The implementation deviates strongly from the previous implementation and fixes a bug in the variance estimation.

The new implementation uses two ValuationResult instances to keep track of the positive and negative running means. After each update, those are combined into the final result object. The update counter of the combined result is set to the minimum of the two update counters. The variance of the combined result is set to the sum of variances (assuming independence).

Open questions

  • How should update counters for MSR be defined? I chose the minimum of the positive and negative update counters because this makes the counters comparable to the counters for normal semivalues.
  • To which value should we set the value estimates if either the positive or the negative mean had zero updates? The paper suggests zero but I think nan is safer. Note that if the update counter is defined as suggested above, this situation can be completely avoided by using MinUpdates as stopping criterion.
  • To which value should we set the variance estimates if either the positive or the negative mean had zero updates? I chose inf.

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Changelog

@janosg janosg changed the base branch from develop to feature/refactor-value June 19, 2024 15:32
@janosg
Copy link
Collaborator Author

janosg commented Jun 24, 2024

I clarified the documentation of ValuationResult.variance, but I still think it is a bit misleading that ValuationResult.variances is not just the square of ValuationResult.stderr.

I think it would be clearer if we only expose the square root of the variances as ValuationResult.stdev; Then it's clear that the difference betweenn stdev and stderr must be a conceptual one. Also, most of the time standard deviations are more interpretable than variances.

Copy link
Collaborator

@jakobkruse1 jakobkruse1 left a comment

Choose a reason for hiding this comment

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

I just looked over the files, seems all good to me at first glance. I did not run any code or tests yet though, so no way to be sure for me. Also the CI is not running for this branch. Are we updating the notebooks for the new valuation structure as well or is this planned at a later stage? Looking at notebooks may help to verify that everything works as expected.
I am not aware of the current plans about notebooks etc. so feel free to ignore me if there are any decisions that I am unaware of.

@janosg janosg requested a review from schroedk July 8, 2024 10:16
@janosg janosg merged commit 1cff43b into feature/refactor-value Jul 9, 2024
@janosg janosg deleted the feature/refactor-msr-banzhaf branch July 9, 2024 08:29
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