Since this is a major change, how about we go up to 1.0?

I'm good with that

There's not test for the function that quantizes on a log scale. But as asked above, I'm not sure if that's actually still needed.

Yeah that's true, there's no tests for it. The parameters that use it are tested though and I figured that's good enough.

What's the reason to change the seeds here?

Whoops might have been through debugging it was left in. No reason it should have been changed at the end.

Do you have any idea why the variance is twice as high as in the previous implementation?

So after some investigation, I found out why, the problem was to do with the test handle it called.

The first part of def _test_get_one_exchange_neighbourhood calculated a num_neighbors which for this uniform int becomes 10.

In the second part, previously it was never passed in and the num_neighbors parameter wasn't used. In this PR, I noticed it wasn't used and must have passed it in as I thought I might have removed it accidentally.

I'm not sure what the intended behaviour is:

1. Do we remove the for loop calculating num_neighbors? In which case, I get something close to the previous mean and variance
2. Do we pass it in? In which case the variance is ~9 as we compute a neighbor configuration for every possible integer.

As a side note, the parameter name is misleading as the num_neighbors parameter to get_one_exchange_neighbourhood is actually the number of neighbor configs per hyperparameter, not the total number of neighbors to retrieve.

The test relies on this function:

def _test_get_one_exchange_neighbourhood(hp):
    cs = ConfigurationSpace()
    num_neighbors = 0
    if not isinstance(hp, list):
        hp = [hp]

    for hp_ in hp:
        cs.add(hp_)
        if np.isinf(hp_.get_num_neighbors()):
            num_neighbors += 4
        else:
            num_neighbors += hp_.get_num_neighbors()

    cs.seed(1)
    config = cs.get_default_configuration()
    all_neighbors = []
    for i in range(100):
        neighborhood = get_one_exchange_neighbourhood(
            config,
            i,
			# Before, num_neighbors was set as blank, which defaulted to 4
            num_neighbors=4,
			# The test after updating had it set to the above `num_neighbors`
			num_neighbors=num_neighbors
        )

@mfeurer this needs a resolution.

Do you have any idea why this value is so much higher than in the previous implementation?

See above

Similar as above, why would the variance increase so much here?

And above

Why did these five values increase by a factor of five?

Ay yes, this I didn't know how to handle properly.

The previous implementation returned the pdf in the value space (even when considering vectorized values).

You can see from the bottom of that function it normalizes with respect to self.upper and self.lower, numbers in value space. In this case reducing it by a factor of 10 (and not 5 as you mentioned).

The newer implementation essentially has hyperparameters being backed by some distribution in vectorized space, that have no idea about what their vectorized format means in value space. The new values reflect this in they not normalized by the lower and upper bound in value space. I figured this was okay as pdf only has a relative meaning when compared to other pdfs, and they all no longer have this value space normalization.

Here's the relevant snippet from the code. I did originally use to pass in the _pdf_norm to rescale things but i figured it was better to have the pdf refer to their pdf in vectorized space and be agnostic to value space.

@dataclass
class ScipyContinuousDistribution(Distribution):
    rv: rv_continuous_frozen

    lower_vectorized: f64
    upper_vectorized: f64
    _max_density: float
    _pdf_norm: float = 1

    def sample_vector(
        self,
        n: int,
        *,
        seed: RandomState | None = None,
    ) -> Array[f64]:
        return self.rv.rvs(size=n, random_state=seed).astype(f64)  # type: ignore

    def max_density(self) -> float:
        return float(self._max_density)

    def pdf_vector(self, vector: Array[f64]) -> Array[f64]:
        pdf = self.rv.pdf(vector) / self._pdf_norm

        # By definition, we don't allow NaNs in the pdf
        return np.nan_to_num(pdf, nan=0)  # type: ignore

As a side beneift, this means its first of all easier to implement new hps as you do not need to consider the interplay of pdf w.r.t. vectorized and value space and secondly if we just rely on the pdf returned by the underlying disitribution, things become a lot more uniform in terms of implementation.

Hey, what's the reason to rename this test and add a number?

Was an issue where two tests in the same file had the same name but were part of test classes. Making it pytest and removing the classes caused it to fail and I think this was me being lazy.

Renamed to test_configuration_space_acts_as_mapping and test_configuration_acts_as_mapping

What's the reason to remove this check?

Seems I removed the these checks in the base Condition class when reworking them. I honestly find crowding every __init__ with several instance(param, ThingIThinkItToBe): raise TypeError("Usually a long message to tell user they called it wrong") to be a bit noisy and provide little value that your editor/type checker can provide before running anything. Exceptions being when the type doesnt convey information, i.e. a float between 0-1.

Also raises the blurry line of which parameters should we check of which objects?
If your set on these being required I can add the checks back in.

Why did you drop this test?

Stray causality, I added it back in and it passes.