PiBO Pull Request 2 - Beta Hyperparameters #222

hvarfner · 2022-01-25T21:17:06Z

PiBO Pull Request 2 - Beta Hyperparameters

Added BetaFloat and BetaInteger HPs

New parameter types
Tests analogous to existing
Write support
Bugfixes from PiBO PR1

for NormalFloat

read_and_write

sampled outside its permitted range.

…king pdf regardlessx of input

Testing left.

hyperparameters.pyx where i changed Typing.

beta and normal distributions

parameters - now, they are generated the same way as the uniform (but scaled, so that it makes sense even though they are not normalized)

hyperparamretersChanged back to standard local search procedure for the normal hyperparamreters

class down to the subclasses Float, Constant, Integer, Categorical and Ordinal Hyperparameters (as these almost certainly can have a well-defined pdf, or that it would make sense to consider a pdf for these HP types).

all the classes that implement a pdf

… betaparams_merge

parameters

for both (identical to Uniform parameters

the other parameter types - follows as closely as possible

hvarfner · 2022-03-03T08:44:10Z

Done. Yeah, I think it turned out better than I first expected. Those small errors are now adressed. Hopefully BetaIntHP doesn't hide any unsuspected surprises.

mfeurer

Alright, I made my way through the tests. They look good, but I have some further comments because I think they can be simplified quite a bit.

Please also pull the latest changes before making any changes yourself because I just fixed a merge conflict with the Web UI.

mfeurer · 2022-03-03T09:26:53Z

test/test_hyperparameters.py

+ # Testing that no error is thrown
+ BetaFloatHyperparameter("param", lower=1, upper=1000.0, alpha=2.0, beta=3.0, q=3.0)
+
+ self.assertAlmostEqual(f5_legal_nolog.default_value, 1)


Why would the default value be one in both settings? Shouldn't the log move the default value?

Maybe we could have a separate test for the default values that checks for log, quantization, and alpha being 1 and beta being one.

The same holds for the integer version. Looking at the code, this is really the only thing that is different than the parent class and that needs to be tested thoroughly

I think it's up to us to decide whether log should move the default value or not. Personally, I think it shouldn't.

Yeah sure, I can split them up and do some more explicit tests for default values.

I think it's up to us to decide whether log should move the default value or not. Personally, I think it shouldn't.

I think diagree on this one. Let's take the example you did below with a hyperparameter from 1 to 100. In the non-log case the default (in my opinion) should be 50, while in the log case it should be 10. In both cases it would be the middle of the distribution.

Oh, on that one I completely agree! I was strictly talking about the case where the user specifies the default, which is the case in the example you brought up.

mfeurer · 2022-03-03T09:28:20Z

test/test_hyperparameters.py

+ f2_actual = f2.to_uniform()
+ self.assertEqual(f2_expected, f2_actual)
+
+ def test_betafloat_is_legal(self):


Don't we inherit is_legal now? Then we wouldn't have to test it, right?

mfeurer · 2022-03-03T09:29:33Z

test/test_hyperparameters.py

+ f1_actual = f1.to_uniform()
+ self.assertEqual(f1_expected, f1_actual)
+
+ def test_betaint_is_legal(self):


Same as above, don't we inherit is_legal?

test/test_hyperparameters.py

mfeurer · 2022-03-03T09:32:43Z

test/test_hyperparameters.py

+ with self.assertRaises(ValueError):
+ BetaIntegerHyperparameter("param", lower=-1, upper=10.0, alpha=6.0, beta=2.0, log=True)
+
+ with self.assertRaisesRegex(ValueError, "Illegal default value 0"):


Not sure if we need to test such things that we inherit from the super class.

Sure, can remove!

mfeurer · 2022-03-03T09:32:59Z

test/test_hyperparameters.py

+ f4_legal_log = BetaIntegerHyperparameter(
+ "param", lower=1, upper=10.0, alpha=3.0, beta=2.0, default_value=1, log=False)
+
+ self.assertAlmostEqual(f4_legal_nolog.default_value, 1)


Same as above, I'm surprised about this.

Same response - I think it's up to us to decide what makes sense here. My reasoning is the following:

If you design a parameter on a log scale - with range from 1-100, with the default in the middle (at 10), you want to pass in 10, and not 1, as the default value - which would be the alternative. I simply think it is the more intuitive way to do it.

hvarfner · 2022-03-03T10:55:21Z

So, what do we still want done? Here's my interpretation:

Remove all BetaHP tests that are strictly inherited from uniform (a no-brainer, I guess). Q: Does this include the neighbor tests?
Make specific parameter tests for alpha and beta
Make specific, and expand, parameter tests for default values

Is that all?

mfeurer · 2022-03-03T12:57:02Z

Remove all BetaHP tests that are strictly inherited from uniform (a no-brainer, I guess). Q: Does this include the neighbor tests?

That is an excellent question, I tthink yes.

Make specific parameter tests for alpha and beta

👍

Make specific, and expand, parameter tests for default values

👍

However, I currently see a different issue with the sampling, and I'm sorry for not seeing that earlier. We need to sample uniformly in [0,1] and then transform into the beta distribution in the transform function. Similarly, the inverse_transform must map into a uniformly distributed [0,1].

hvarfner · 2022-03-03T14:11:07Z

However, I currently see a different issue with the sampling, and I'm sorry for not seeing that earlier. We need to sample uniformly in [0,1] and then transform into the beta distribution in the transform function. Similarly, the inverse_transform must map into a uniformly distributed [0,1].

I'm not sure I get it. From my own tests, it seems to sample as intended. Can you specify?

I'm on the rest, it should be done in a couple of hours.

mfeurer · 2022-03-03T16:12:44Z

Yes, it samples correctly, but I think the behavior is still incorrect. In the unit hypercube everything should be uniformly distributed, and we then transform into the desired distribution, similar to the idea of inverse transform sampling. By guaranteeing that the unit hypercube is uniform, all models can work with the data out of the box, and procedures like the neighborhood generation work the same for algorithm.

As before, I'm sorry that this is not yet documented and that it didn't come up before, and we need to improve on this.

hvarfner · 2022-03-03T16:17:20Z

Sorry, but I still don't really follow what behavior you are referring to!

mfeurer · 2022-03-03T16:50:27Z

Unfortunately, such behavior is not implemented yet. I think the required changes would be:

Sample from a uniform distribution in _sample (probably inherited).
In transform you need to map to a beta distribution, I think this is done with sp_beta.ppt()
In inverse_transform you need to map from beta back to uniform and this can be done with sp_beta.cdf()

The outcome of sampling from the distribution should remain the same, but the meaning of the transformed values would change.

hvarfner · 2022-03-03T16:58:38Z

No, I disagree. This would equate to warping the entire search space like in this, relatively famous paper:
https://arxiv.org/abs/1402.0929

That is not what I sought out to do here. Neither is this the case for the normalHPs that are already implemented.

hvarfner · 2022-03-03T17:01:27Z

If one wanted to leave the door open for that to be a possibility, you would have to reconsider the transforms. If that's the case, however, the Beta cannot inherit from the uniform in any way whatsoever. Maybe you pointed this out some time ago, but if not, I'm doing it now!

hvarfner · 2022-03-03T17:04:40Z

So, all in all. I personally am not looking to warp the search space, but only to have a parameter which one can sample from and evaluate the pdf of the accompanying beta distribution. These two things do not really go hand in hand as things are currently designed.

mfeurer · 2022-03-03T17:15:16Z

This would equate to warping the entire search space like in this, relatively famous paper:

Yes, I fully agree on that.

Neither is this the case for the normalHPs that are already implemented.

They are unbounded, so we cannot really normalize them. For the bounded ones this is in my opinion not implemented correctly at the moment and should be implemented to the warped version, too.

So, all in all. I personally am not looking to warp the search space, but only to have a parameter which one can sample from and evaluate the pdf of the accompanying beta distribution. These two things do not really go hand in hand as things are currently designed.

Hm, maybe we should follow the rather pragmatic approach of getting this merged. It right now does the right thing and so having it in is definitely a huge step forward. Formalizing whether we should be in a uniformly distributed hypercube or just a hypercube is something we can do later and that should involve the other stakeholders and users of the ConfigSpace, too.

default values. Notably, quantization plus logging does not yield correct default values (i.e., not the mode of the distribution) all the time, as there is some undesired adjustment of the bounds ._lower and ._upper in UniformFloatHyperparameter which skews it.

into betaparameters

hvarfner · 2022-03-03T17:22:50Z

I pulled the latest changes and got quite a lot of failed tests (3 of them). Do you have this, too?

mfeurer · 2022-03-03T17:43:11Z

Tests appear to work in CI. Documentation building will be fixed in another PR. However, formatting doesn't look good yet, could you please apply the make format as you did for the PR before?

hvarfner · 2022-03-03T17:47:55Z

Absolutely. The Integer default tests are still to go (though they are almost 100% reliant on the float ones, so they can be a lot shorter).

I think that's a good approach, let's do that!

mfeurer · 2022-03-03T17:53:26Z

Absolutely. The Integer default tests are still to go (though they are almost 100% reliant on the float ones, so they can be a lot shorter).

Okay, please just give me a go when I shall have a final look at the tests then (preferably with the pre-commit having a positive status, too).

hvarfner · 2022-03-03T19:06:43Z

@mfeurer Okay, now I am fairly optimistic that we're done with this one. There was one more issue that arose:

When doing both quantization and logging simultaneously, the default values of the beta are not computed correctly. This is because the bounds of the normal, ._lower and ._upper, which the transformation relies on, get expanded by 0.5 in each direction in non-logspace. When the default is in the center of the search space (like with uniforms), but it is not when the default is somewhere else in the interior.

This issue is likely insignificant, and it is an unintended consequence of inheriting from UniformFHP. The easy solution is for the user to specify a default when using BetaHP+log+quant+no default. Added a warning in BetaFloat specifically for this case.

hvarfner · 2022-03-03T19:08:03Z

Notably, I do not test for that niche case either, as any test values would simply be incorrect in terms of the actual computation. If you want, I can elaborate on this more, but just know that it is only relevant in this specific setting of log+quant (or int) + no default.

mfeurer

Looks good to me. I think I only found one minor thing: a comment which was accidentally copied over.

test/test_hyperparameters.py

Co-authored-by: Matthias Feurer <lists@matthiasfeurer.de>

hvarfner · 2022-03-03T19:45:10Z

Awesome! it is now changed, too. I will jump on the last PR and integrate all that's been discussed. However, most of it has been kept up to speed with the rest.

mfeurer · 2022-03-03T19:59:37Z

Great, just merged!

@eddiebergman as this PR no longer includes the neighborhood generation, we should follow up on that in a separate thread/PR.

I'll also post an issue to discuss the hypercube scaling/warping.

hvarfner added 30 commits December 7, 2021 14:15

Got structure and notes in place to implement PiBO - commenting and pdf

17e28ad

for NormalFloat

Added class BetaFloatHyperparameter, and allowed for its support in

283415b

read_and_write

Fixed bug where a NormalFLoatHyperparameter in log space could be

d8803d5

sampled outside its permitted range.

Checked normalfloatparameter with inputs - now has a complete and wor…

641129a

…king pdf regardlessx of input

Implemented BetaIntegerHyperparameter - onlb brief testing conducted

7a168b5

Finished categorical hyperparameter and pdf for all hyperparameters.

ba38e01

Testing left.

Added to_uniform for categorical (to uniform weights

5b7c2e4

Added to_uniform in configspace, fixed some earlier mistakes in

24686c7

hyperparameters.pyx where i changed Typing.

Removed debugging method "get_probs"

1aa4cce

Fixed bug in beta neighborhood generation - still have not normalized

7eb9ecb

beta and normal distributions

Changed neighbor generation to be consistent for Normal and Beta

cec5645

parameters - now, they are generated the same way as the uniform (but scaled, so that it makes sense even though they are not normalized)

Changed back to standard local search procedure for the normal

d862ff3

hyperparamretersChanged back to standard local search procedure for the normal hyperparamreters

Forgot to change back neighbor generation for normal float

10de917

Removed done TODO's and commentary

c80f0da

Added return types for .pdf and removed a TODO

0df4b19

Written user guide for placing priors on parameters and the pdf method

0ab46f4

Reformatted some faulty things with the user guide

0b72162

One more user guide fix

1cd34b5

Fixed a few bugs in check_default, and added method get_max_density for

d04eee9

all the classes that implement a pdf

Found a bug in normalfloat and removed a TODO

e4f268f

Merge branch 'master' of https://github.com/hvarfner/ConfigSpace into…

a1c225e

… betaparams_merge

Completing merge with master - not implemented get_size for beta

db1692d

parameters

Changed name of to_uniform and minor fixes

4030097

Corrected for wrong placement in merge

9e6558c

Changed minor bugs in the beta parameters and added the get_size methods

0d8622e

for both (identical to Uniform parameters

Added unit tests for betafloathyperparameter - in line with the ones for

4162900

the other parameter types - follows as closely as possible

Added additional bugfixes to beta parameters

71e44e7

Added tests to BetaIntegerParameters

be6add8

Added skeleton of all remaining tests

6ee9c55

Merge branch 'master' into betaparameters

f8bfec1

mfeurer reviewed Mar 3, 2022

View reviewed changes

hvarfner added 2 commits March 3, 2022 18:18

Merge branch 'betaparameters' of https://github.com/hvarfner/ConfigSpace

ad769b0

into betaparameters

hvarfner added 2 commits March 3, 2022 19:50

Fixed all the necessary tests for BetaHPs

25424ab

Formatted, pre-commit now passes.

33803ea

mfeurer approved these changes Mar 3, 2022

View reviewed changes

test/test_hyperparameters.py Outdated Show resolved Hide resolved

Update test/test_hyperparameters.py

b869d14

Co-authored-by: Matthias Feurer <lists@matthiasfeurer.de>

mfeurer merged commit 4cc7bde into automl:master Mar 3, 2022

eddiebergman mentioned this pull request May 6, 2022

[Optimization] get_neighbours can generate n samples and then validate them #236

Closed

mfeurer mentioned this pull request May 6, 2022

Unification and documentation of internal representation of hyperparameters #252

Closed

PiBO Pull Request 2 - Beta Hyperparameters #222

PiBO Pull Request 2 - Beta Hyperparameters #222

Conversation

hvarfner commented Jan 25, 2022

PiBO Pull Request 2 - Beta Hyperparameters

Added BetaFloat and BetaInteger HPs

hvarfner commented Mar 3, 2022

mfeurer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022

hvarfner commented Mar 3, 2022

However, I currently see a different issue with the sampling, and I'm sorry for not seeing that earlier. We need to sample uniformly in [0,1] and then transform into the beta distribution in the transform function. Similarly, the inverse_transform must map into a uniformly distributed [0,1].

mfeurer commented Mar 3, 2022

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022

hvarfner commented Mar 3, 2022

hvarfner commented Mar 3, 2022

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022 • edited Loading

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022

hvarfner commented Mar 3, 2022

hvarfner commented Mar 3, 2022

mfeurer left a comment

Choose a reason for hiding this comment

hvarfner commented Mar 3, 2022

mfeurer commented Mar 3, 2022

mfeurer commented Mar 3, 2022 •

edited

Loading