Hyperparameter PDFs an Densities by hvarfner · Pull Request #241 · automl/ConfigSpace

hvarfner · 2022-03-07T08:44:17Z

PiBO Pull Request 3 remake - PDFs and Densities

Third pull request - Implements _pdf, pdf and get_max_density for each parameter type, as well as other support needed for PiBO

Everything from PiBO PR1 and PR2
_pdf and pdf
get_max_density
remove_parameter_priors with supporting static methods which copy forbiddens and conditions from the old (non-uniform) configspace to the new one
- static method substitute_hyperparameters_in_conditions()
- static method substitute_hyperparameters_in_forbiddens()
Accompanying tests
Documentation in user guide
Betaparameter pdfs now consider Unit hypercube scaling (plus skewing induced by Integer/quantization)

codecov · 2022-03-07T08:50:19Z

Codecov Report

Merging #241 (1e77c8e) into master (4cc7bde) will increase coverage by 4.64%.
The diff coverage is n/a.

❗ Current head 1e77c8e differs from pull request most recent head 4538ff4. Consider uploading reports for the commit 4538ff4 to get more accurate results

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   62.43%   67.07%   +4.64%     
==========================================
  Files          17       17              
  Lines        1637     1637              
==========================================
+ Hits         1022     1098      +76     
+ Misses        615      539      -76

Impacted Files	Coverage Δ
ConfigSpace/read_and_write/pcs_new.py	`90.93% <0.00%> (+8.47%)`	⬆️
ConfigSpace/read_and_write/pcs.py	`85.53% <0.00%> (+19.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cc7bde...4538ff4. Read the comment docs.

eddiebergman · 2022-03-07T12:21:18Z

Hi @hvarfner,

The failing test seems to be a slight floating point numerical difference. You can use assert_allclose from numpy to fix this.

I'll do a full review now :)

eddiebergman

Minor comments, I havn't checked the actual correctness of the pdf implementations but I'm assuming with your tests that it is correct :)

ConfigSpace/hyperparameters.pyx

eddiebergman · 2022-03-07T12:41:54Z

ConfigSpace/hyperparameters.pyx

+        Parameters
+        ----------
+        vector: np.ndarray
+            the (N, ) vector of imputs for which the probability density


Just noticed the word imput, should be input. Seems the same in all other doc strings, if you have time, a search-replace over imputs would be nice :)

ConfigSpace/hyperparameters.pyx

docs/source/User-Guide.rst

test/test_configuration_space.py

hvarfner · 2022-03-07T13:35:47Z

Cool, thanks! I'll address everything you found later thi afternoon!

And to perhaps make you feel a bit at ease - Making sure that the pdfs were correctly computed was my absolute biggest timesink, so I'm pretty confident that they are correct!

hvarfner · 2022-03-08T08:39:08Z

Okay, new pass done. I hope everything is adressed once again. I did change the index.rst file too, as the added weights to the CategoricalHPs end up changing the RNG.

LMK if there's anything else that needs to be done!

eddiebergman · 2022-03-08T14:07:43Z

Hi @hvarfner,

I'm happy with the changes and happy to see all the green ticks :) I would ask @mfeurer to take a look which I assume will be towards the end of the week or next. Sorry for the delay on that.

We really do appreciate all the patience you've had to get this implemented. It was a large change and dealt with fixing a few bugs that were present. Many kudos!

I think it highlights some areas of improvements required from us, notably in my view:

Documentation of what's expected by implemented Hyperparamters
Slight optimizations for sampling/neighbours
I would argue the hyperparameters should be split into seperate files at this point.
Rethink class heriarchy
Use properties for internal object properties, removes a lot of the if/else checking if transformed or not.

We also briefly discussed in side channels of having the distributions be a component of a class, rather than baking it into the class itself. That would make things a lot more flexible and testable.

If you have further thoughts on specific improvements, they would be greatly appreciated!

Best,
Eddie

hvarfner · 2022-03-08T22:58:33Z

Many thanks to you too, Eddie - and obviously Matthias as well!

Great to hear that your experience was pleasant, as mine was, too. Obviously, it took quite a lot of patience on you guys' end to get all of my quirks sorted out.

I think your bullets make sense. My impression is that there were one or two crossroads in terms of design, where I went with my gut and we had to backtrack. For example, the various domains that parameters work with internally and the intricacies of _transform and _inverse_transform.

I was rather surprised when I first saw the domain that a logged IntegerHP works with internally, and it took me a while to figure out. If I had one improvement, it would probably be that all HPs work with a strict Unit Hypercube internally (self._lower=0, lowest value in domain=0), whether they are discrete/discretized, Categorical - you name it. I do believe this would assist in implementing additional features in the future, and help substantially in testing private methods.

I would agree with the splitting and neighbors point as well.

On the question of class hierarchy, I have not given this a lot of though myself, but I feel like what's in place now is a very valid approach. Whether there's a better one out there, I'm not sure. However, I'd be happy to discuss if you want my input!

mfeurer

Hi folks,

I just had a quick look over the implementation and it looks fine overall. I didn't have the time to look into the unit tests, though, but please go ahead with this PR without me doing so. WRT to @eddiebergman's comments:

Documentation of what's expected by implemented Hyperparamters

Yes! I promised to open an issue to spawn a discussion on the scaling/warping of continuous values, but I'm afraid I will only be able to do so in April. @hvarfner if you want to go ahead on this, please do so :) However, I'm not sure if we can/should map categoricals between 0/1, though.

Slight optimizations for sampling/neighbours

That sounds great!

I would argue the hyperparameters should be split into seperate files at this point.

Totally agree on that. Compiling now takes ages if you just change one HP

Rethink class heriarchy

Yes!

Use properties for internal object properties, removes a lot of the if/else checking if transformed or not.

Maybe, we always need to check whether such Python additions are equally fast in Cython.

We also briefly discussed in side channels of having the distributions be a component of a class, rather than baking it into the class itself. That would make things a lot more flexible and testable.

That would be an amazing option. I'm not sure if we can/should do this for all distributions (because their implementation might slow down sampling), but we should have this as a general option.

ConfigSpace/configuration_space.pyx

mfeurer · 2022-03-11T11:42:20Z

ConfigSpace/configuration_space.pyx

+
+        Parameters
+        ----------
+        new_configspace: ConfigurationSpace 


The return type is incorrect.

Return type? Are you referring to the list of conditions? I couldn't find anything wrong, so what are you referring to?

mfeurer · 2022-03-11T11:42:35Z

ConfigSpace/configuration_space.pyx

            return size

+    @staticmethod
+    def substitute_hyperparameters_in_conditions(conditions, new_configspace):


Could you please add type hints?

mfeurer · 2022-03-11T11:43:02Z

ConfigSpace/configuration_space.pyx

+        return new_conditions
+
+    @staticmethod
+    def substitute_hyperparameters_in_forbiddens(forbiddens, new_configspace):


Same issue as above (also the return type).

ConfigSpace/hyperparameters.pyx

shrunk unit hypercube range for the Beta hyperparameter case, and tests have been rewritten to accomodate.

hvarfner · 2022-03-15T09:50:21Z

@eddiebergman
Now, all of Matthias' last proposed edits are incorporated, everything squashed, and all the tests should pass. Can we merge?

eddiebergman · 2022-03-15T12:52:04Z

Hi @hvarfner, Seems like the tests are passing, I'll just wait till they're all done and then I'll merge it in :)

hvarfner · 2022-03-21T12:27:48Z

@eddiebergman ping
=)

eddiebergman · 2022-03-21T12:29:53Z

Good ping, my bad!

hvarfner · 2022-03-21T12:30:43Z

Thanks!

…rks with the (#241)

eddiebergman reviewed Mar 7, 2022

View reviewed changes

eddiebergman approved these changes Mar 8, 2022

View reviewed changes

mfeurer reviewed Mar 11, 2022

View reviewed changes

Squashed all the changes related to densities. _pdf works with the

4538ff4

shrunk unit hypercube range for the Beta hyperparameter case, and tests have been rewritten to accomodate.

hvarfner force-pushed the pdf branch from 1e77c8e to 4538ff4 Compare March 15, 2022 09:49

eddiebergman merged commit 175f798 into automl:master Mar 21, 2022

github-actions bot pushed a commit that referenced this pull request Mar 21, 2022

Carl Hvarfner: Squashed all the changes related to densities. _pdf wo…

39bf25f

…rks with the (#241)

Conversation

hvarfner commented Mar 7, 2022

PiBO Pull Request 3 remake - PDFs and Densities

Third pull request - Implements _pdf, pdf and get_max_density for each parameter type, as well as other support needed for PiBO

Uh oh!

codecov bot commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eddiebergman commented Mar 7, 2022

Uh oh!

eddiebergman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eddiebergman Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

hvarfner Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvarfner commented Mar 7, 2022

Uh oh!

hvarfner commented Mar 8, 2022

Uh oh!

eddiebergman commented Mar 8, 2022

Uh oh!

hvarfner commented Mar 8, 2022

Uh oh!

mfeurer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mfeurer Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

hvarfner Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

mfeurer Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

mfeurer Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvarfner commented Mar 15, 2022

Uh oh!

eddiebergman commented Mar 15, 2022

Uh oh!

hvarfner commented Mar 21, 2022

Uh oh!

eddiebergman commented Mar 21, 2022

Uh oh!

hvarfner commented Mar 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 7, 2022 •

edited

Loading