[Tune] Fix and optimize sample search algorithm quantization logic

[ilee300a]

Why are these changes needed?

Current quantization logic incorrectly rounds sampled values. In this PR, this error is fixed and quantization for q = 1 is optimized by omitting quantization.

Related issue number

Closes #28045

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[xwjiang2010]

I was wondering maybe an example is easier to understand than wording?
e.g. the qrandint(1, 10, 3) translates to choosing randomly among [3, 6, 9] as we were discussing yesterday?

[krfricke]

It should choose between [1, 4, 7, 10] though, right?

[ilee300a]

The quantized sampling rounds the sampled values to multiples of q rather than integer increment of q from the lower bound. So qrandint(1,10,3) will randomly sample from [3,6,9] rather than [1,4,7,10]

[xwjiang2010]

@ilee300a do you want to provide more details as to why the decision was made so? My understanding is there is some impl that is shared with other sampling methods, which may require [3, 6, 9] instead of [1, 4, 7, 10].

[ilee300a]

When using other libraries, the expected behavior of quantization is to select those that are divisible by the given q within the bounds. For example, when using BOHB, it expects the provided lower and upper bound to be a multiple of q and provides multiples of q within those bounds as the samples. So if we run qrandint(1,10,3), it actually samples multiples of 3 between 3 and 9(inclusive).

[Yard1]

Makes me wonder if we should raise an exception during domain initialisation if the bounds are not divisible instead. Or at least a warning - if we silently change the bounds it may be surprising to some users

[xwjiang2010]

Thanks! Left some comment..

[xwjiang2010]

Could you also rebase/merge on the current upstream/master? That should hopefully fix the tests :)

[Yard1]

shouldn't we deepcopy the existing domain instead of creating a new one? seems like we are losing subclass type here.

[krfricke]

Thanks!

Can we add some test cases here (in test_sample.py):

self.config = {
        # ...
            "qrandint": tune.qrandint(-21, 12, 3),
            "qrandint_q3": tune.qrandint(1, 10, 3),
            "qrandint_q1": tune.qrandint(1, 5, 1),

            if "qrandint_q3" not in ignore:
                self.assertGreaterEqual(out["qrandint_q3"], 1)
                self.assertLessEqual(out["qrandint_q3"], 10)
                self.assertEqual(out["qrandint_q3"] % 3, 0)
                self.assertTrue(isinstance(out["qrandint_q3"], int))

            if "qrandint_q1" not in ignore:
                self.assertGreaterEqual(out["qrandint_q1"], 1)
                self.assertLessEqual(out["qrandint_q1"], 5)
                self.assertEqual(out["qrandint_q1"] % 1, 0)
                self.assertTrue(isinstance(out["qrandint_q1"], int))

                elif k == "qrandint_q3":
                    for i in range(1, 10, 3):
                        self.assertIn(i, v, msg=f"qrandint_q3 failed for i={i}")

                elif k == "qrandint_q1":
                    for i in range(1, 5, 1):
                        self.assertIn(i, v, msg=f"qrandint_q1 failed for i={i}")

Thanks!

[krfricke]

LGTM! Thanks!