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

Remove default non-negativity constraint on least core subsidy #304

Merged

Conversation

AnesBenmerzoug
Copy link
Collaborator

@AnesBenmerzoug AnesBenmerzoug commented Mar 8, 2023

Description

This PR closes #294

Changes

  • Add non_negative_subsidy flag to least core methods with a default value of False.
  • Remove empty subset from lower bound constraints, if found, because it is the same as using the constraint $e >= 0$ if $u(\emptyset) = 0$ (which is true in almost all cases) (i.e. setting non_negative_subsidy=True)
  • Remove total utility from lower bound constraints, if found, because it is the same as using the constraint $e >= 0$ (i.e. setting non_negative_subsidy=True)
  • Rename options to solver_options.
  • Pass solver_options as dictionary instead of as kwargs.
  • Shows a deprecation warnings when passing solver options as kwargs.
  • Change default solver from ECOS to SCS.
  • Set default solver max iterations to 10000.
  • Update least core notebook.
  • Update core and least core docs' constraints to use proper non-empty subsets.

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Documentation (if necessary)
  • Updated Changelog
  • If notebooks were added/changed, added boilerplate cells are tagged with "nbsphinx":"hidden"

@AnesBenmerzoug AnesBenmerzoug self-assigned this Mar 8, 2023
@AnesBenmerzoug AnesBenmerzoug marked this pull request as ready for review March 8, 2023 18:22
@mdbenito
Copy link
Collaborator

mdbenito commented Mar 8, 2023

(without looking at the PR) Note that the constraint for the empty set only implies e > 0 if u(S)=0

@mdbenito
Copy link
Collaborator

mdbenito commented Mar 8, 2023

And, since you update the docs, would it be possible to keep the notation consistent with the rest of the docs? i.e. identify points $x_i$ with their indices $i$ and use $v_i$ instead of $v_u(x_i)$

@AnesBenmerzoug
Copy link
Collaborator Author

@mdbenito I actually used the same notation as in the rest of the data valuation documentation.

Do you mean only in the docstring? If not, then I would suggest we leave that as a separate issue and discuss it tomorrow.

@mdbenito
Copy link
Collaborator

mdbenito commented Mar 9, 2023

@mdbenito I actually used the same notation as in the rest of the data valuation documentation.

My bad, it's the rest of the doc that's quite inconsistent.

Do you mean only in the docstring? If not, then I would suggest we leave that as a separate issue and discuss it tomorrow.

See #308

Copy link
Collaborator

@mdbenito mdbenito left a comment

Choose a reason for hiding this comment

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

Just had a couple of questions. Otherwise this looks good. We should repeat the experiments, btw...

src/pydvl/value/least_core/__init__.py Outdated Show resolved Hide resolved
src/pydvl/value/least_core/common.py Outdated Show resolved Hide resolved
src/pydvl/value/least_core/common.py Outdated Show resolved Hide resolved
src/pydvl/value/least_core/common.py Outdated Show resolved Hide resolved
src/pydvl/value/least_core/common.py Show resolved Hide resolved
@mdbenito mdbenito self-requested a review March 10, 2023 13:46
@mdbenito mdbenito merged commit b4305b4 into develop Mar 10, 2023
@mdbenito mdbenito deleted the 294-remove-unnecessary-constraint-on-least-core-subsidy branch March 10, 2023 13:46
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.

Remove unnecessary constraint on Least Core subsidy
2 participants