Skip to content

Allow fixed entries in probability constraints#683

Open
hmgaudecker wants to merge 4 commits into
mainfrom
probability-allow-fixed-entries
Open

Allow fixed entries in probability constraints#683
hmgaudecker wants to merge 4 commits into
mainfrom
probability-allow-fixed-entries

Conversation

@hmgaudecker

@hmgaudecker hmgaudecker commented Apr 30, 2026

Copy link
Copy Markdown
Member

In an application, I need probability constraints where one element is fixed. This is currently rejected by optimagic; this PR fixes that.

hmgaudecker and others added 3 commits April 22, 2026 10:49
Consolidate fixed-at-zero entries out of a ProbabilityConstraint before the
kernel transformation runs, so a FixedConstraint pinning some selected
entries to 0.0 can coexist with a ProbabilityConstraint on the same
parameters. The zero-fixed entries are driven by the existing fixed-value
pipeline; the remaining free entries form a simplex summing to one.

Fixes to values other than 0.0 still raise InvalidConstraintError; that
generalisation is left for a follow-up commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generalise the zero-fix fold: any fix in [0, 1) whose sum over the
selector is strictly less than 1 is accepted. The fold attaches
sum_target = 1 - sum(fixed_values) to the transformation dict, and
probability_from_internal and its Jacobian scale by sum_target. The pure
zero-fix path is unchanged (sum_target key omitted, semantics identical
to the no-fix path).

probability_to_internal stays untouched because x / x[-1] is scale
invariant; the internal pivot is still 1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The zero-fix and non-zero-fix changes ship together in one PR; one
bullet describes the end-state behaviour.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hmgaudecker
hmgaudecker requested a review from janosg April 30, 2026 16:22
@codecov

codecov Bot commented Apr 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/optimagic/parameters/check_constraints.py 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
...rc/optimagic/parameters/consolidate_constraints.py 96.92% <100.00%> (+0.43%) ⬆️
src/optimagic/parameters/kernel_transformations.py 99.18% <100.00%> (+0.01%) ⬆️
src/optimagic/parameters/check_constraints.py 95.79% <0.00%> (+8.40%) ⬆️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timmens timmens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Found one functional issue: folding a fixed original pivot could leave a zero free entry as the new pivot, causing division by zero in probability_to_internal. Commit 005ea3a selects a positive free pivot when required and adds focused plus end-to-end regression coverage. It also corrects the Jacobian formula documentation.

Comment on lines +366 to +368
assert res.params[0] == 0.0
assert res.params[2] == 0.0
aaae(res.params[[1, 3, 4]].sum(), 1.0)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The first three checks are redundant since the last checks the exact values of the result vector.


res = minimize(
fun=criterion,
params=np.array([0.2, 0.3, 0.2, 0.3]),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The start vector shoudl be harder, i.e. more different from the target.

Comment on lines +389 to +390
assert res.params[0] == 0.2
aaae(res.params[1:].sum(), 0.8)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant

assert res.params[0] == 0.2
aaae(res.params[1:].sum(), 0.8)
aaae(res.params, [0.2, 0.2, 0.3, 0.3], decimal=4)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's add one test case with more than one fixed parameter

)


def test_probability_constraint_with_zero_fix_on_selector_element():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In all of the tests below the probability constraint is not really binding because the unconstrained optimum of the function happens to satisfy the constraint. We need harder test cases!

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.

3 participants