Skip to content

Fixing one off in the uniform_on_restart bug.#207

Closed
kieronqtran wants to merge 2 commits into
Gecode:release/6.3.0from
kieronqtran:fix_uniform_on_restart
Closed

Fixing one off in the uniform_on_restart bug.#207
kieronqtran wants to merge 2 commits into
Gecode:release/6.3.0from
kieronqtran:fix_uniform_on_restart

Conversation

@kieronqtran

@kieronqtran kieronqtran commented Oct 17, 2025

Copy link
Copy Markdown

This pull request makes a small but important fix to the random value assignment logic in the gecode/flatzinc/flatzinc.cpp file. The change ensures that the generated random integer values can include the upper bound of the specified range, correcting an off-by-one error. This is happened becasue Rnd _random input domain is [lower_bound, upper_bound) and + 1 to correct set to [lower_bound, upper_bound], and it is effect to uniform_on_restart constraints.

  • Fixed the calculation of random values for uniform integer ranges to include the upper bound by changing the random range from (range.second - range.first) to (range.second - range.first + 1).

@Dekker1

Dekker1 commented Oct 26, 2025

Copy link
Copy Markdown
Contributor

This fix looks good to me. As you pointed out, I didn't see that the Rnd::operator() gives a value in the range [0,n) instead of [0,n] as I expected.

The same issue actually is also present for floating point, although in a different form:

const FloatVal rndVal = (static_cast<FloatVal>(_random(INT_MAX)) / INT_MAX)*(range.second - range.first) + range.first;

This will never reach the upper bound, as the division will never reach one because INT_MAX is excluded. To fix this, we might have to give up some of the granularity in the division and reduce the right-hand side of the division:

const FloatVal rndVal = (static_cast<FloatVal>(_random(INT_MAX)) / (INT_MAX-1))*(range.second - range.first) + range.first;

This seems fine, and I think any other fix would require an inclusive version of the random number generator, which would be more work.

@kieronqtran

Copy link
Copy Markdown
Author

Updated for uniform_on_researt for float. Thank @Dekker1

@zayenz zayenz mentioned this pull request Jul 7, 2026
@zayenz

zayenz commented Jul 7, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution, and sorry this sat open for so long.

I reviewed this against the current main branch and ported the underlying fix as a fresh commit, with a small modernization to avoid integer overflow on wide ranges and with regression coverage for the FlatZinc on_restart path. The changelog entry credits your original PR.

I’m closing this PR as superseded by PR #214 . Thank you for finding and fixing the endpoint bug.

@zayenz zayenz closed this Jul 7, 2026
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