Skip to content

FIX SAGA solver ignoring sample_weight when drawing samples (gh-21305) - #34799

Open
Boubker10 wants to merge 1 commit into
scikit-learn:mainfrom
Boubker10:fix-saga-sample-weight-sampling
Open

FIX SAGA solver ignoring sample_weight when drawing samples (gh-21305)#34799
Boubker10 wants to merge 1 commit into
scikit-learn:mainfrom
Boubker10:fix-saga-sample-weight-sampling

Conversation

@Boubker10

Copy link
Copy Markdown

Reference Issues/PRs

Fixes #21305

What does this implement/fix? Explain your changes.

SAG/SAGA draw one sample per inner iteration via
SequentialDataset._get_random_index(), which is uniform over the rows and
never accounts for sample_weight -- sample_weight only scaled the loss of
whichever sample was drawn, not the probability of drawing it. When weights
are far from uniform this makes the stochastic gradient estimate very
high-variance and the solver converges poorly or not at all.

Since sample_weight is fixed for the duration of a solver run, this PR
builds a Walker/Vose alias table once (O(n_samples)) so each iteration can
draw a sample proportional to its weight in O(1), and applies the matching
inverse-probability correction to SAGA's direct gradient term to keep the
estimator unbiased (the sum_gradient/`intercept_
are deterministic bookkeeping and must keep accumulating the raw, unscaled
delta).

Restricted to SAGA (is_saga=True): plain SAG has
correction and relies on every sample being revisited regularly, so skewing
the draw towards heavy samples can slow it down in
separate, pre-existing limitation this does not address. Highly skewed
weights combined with L1's proximal operator can a
the default max_iter to satisfy the strict tol criterion, though the
reached solution is markedly better than before at
budget.

No-op for uniform sample_weight (including the default, None): the
original code path is taken unchanged, verified bi
random_state.

@github-actions

Copy link
Copy Markdown

Thank you for opening your first pull request to scikit-learn! 🎉

To help get your contribution reviewed, please make sure that:

  • You have filled out the pull request template.

  • The pull request addresses an existing issue that is ready for contribution (e.g. not tagged as 'Needs Triage', 'Needs Decision', ...). If you are proposing a new feature, please open an issue to discuss it first.

  • There are no other open pull requests already targeting the same issue.

  • You have followed the pull request checklist. In particular, linting and tests should pass.

@github-actions github-actions Bot added the CI:Linter failure The linter CI is failing on this PR label Aug 23, 2026
@Boubker10
Boubker10 force-pushed the fix-saga-sample-weight-sampling branch from cb52bfa to 2919ccf Compare August 25, 2026 19:42
@github-actions github-actions Bot removed the CI:Linter failure The linter CI is failing on this PR label Aug 25, 2026
…earngh-21305)

SAG/SAGA draw one sample per inner iteration via
SequentialDataset._get_random_index(), which is uniform over the rows and
never accounts for sample_weight -- sample_weight only scaled the loss of
whichever sample was drawn, not the probability of drawing it. When
weights are far from uniform this makes the stochastic gradient estimate
very high-variance and the solver converges poorly or not at all.

Since sample_weight is fixed for the duration of a solver run, build a
Walker/Vose alias table once (O(n_samples)) so each iteration can draw a
sample proportional to its weight in O(1), and apply the matching
inverse-probability correction to SAGA's direct gradient term to keep the
estimator unbiased (the sum_gradient/intercept_sum_gradient accumulators
are deterministic bookkeeping and must keep accumulating the raw,
unscaled delta).

Restricted to SAGA (is_saga=True): plain SAG has no per-draw bias
correction and relies on every sample being revisited regularly, so
skewing the draw towards heavy samples can slow it down instead of
helping -- a separate, pre-existing limitation this does not address.
Highly skewed weights combined with L1's proximal operator can also still
need more than the default max_iter to satisfy the strict tol criterion,
though the reached solution is markedly better than before at any given
iteration budget.

No-op for uniform sample_weight (including the default, None): the
original code path is taken unchanged, verified bit-identical for a
fixed random_state.
@Boubker10
Boubker10 force-pushed the fix-saga-sample-weight-sampling branch from 2919ccf to 3efa3fb Compare August 25, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LogisticRegression with SAGA using sample_weight does not converge

1 participant