Skip to content
This repository was archived by the owner on Feb 28, 2024. It is now read-only.

[MRG] Added checking-functions for clarity and reusability. #597

Closed
Hvass-Labs wants to merge 1 commit into from
Closed

[MRG] Added checking-functions for clarity and reusability. #597

Hvass-Labs wants to merge 1 commit into from

Conversation

@Hvass-Labs
Copy link
Contributor

@Hvass-Labs Hvass-Labs commented Jan 8, 2018

This is a quick PR which moves some exception-handling into separate functions for clarity and reusability. It currently only affects @use_named_args() that I recently added.

I searched for the proper module to place these in and noticed that utils.py already has check_x_in_space() so I placed them in utils.py as well.

It can be tested with the following code:

from skopt.space import Real, Integer, Dimension
from skopt.utils import check_list_types

dim1 = Real(name='foo', low=0.0, high=1.0)
dim2 = Real(name='bar', low=0.0, high=1.0)
dim3 = Real(name='baz', low=0.0, high=1.0)

dimensions = [dim1, dim2, dim3, 'Hello']

check_list_types(dimensions, (Integer, Dimension))

This fails and prints:

ValueError: All elements in list must be instances of (<class 'skopt.space.space.Integer'>, <class 'skopt.space.space.Dimension'>), but found: ['Hello']

And the following code:

from skopt.space import Real
from skopt.utils import check_dimension_names

dim1 = Real(name='foo', low=0.0, high=1.0)
dim2 = Real(name=None, low=0.0, high=1.0)
dim3 = Real(name='baz', low=0.0, high=1.0)

dimensions = [dim1, dim2, dim3]

check_dimension_names(dimensions)

This fails and prints:

ValueError: All dimensions must have names, but found: [Real(low=0.0, high=1.0, prior='uniform', transform='identity')]

Note that PR #579 changes the semantics of dimension-names, so that all dimensions are now given default names if the user has not supplied a name, so you can argue this check is not really needed after that. But there is an ongoing discussion about naming dimensions, so I think it is wise to keep this check for now.

@pep8speaks
Copy link

pep8speaks commented Jan 8, 2018

Hello @Hvass-Labs! Thanks for submitting the PR.

Line 508:1: W293 blank line contains whitespace
Line 511:1: W293 blank line contains whitespace

@Hvass-Labs Hvass-Labs changed the title Added checking-functions for clarity and reusability. [MRG] Added checking-functions for clarity and reusability. Jan 8, 2018
@Hvass-Labs
Copy link
Contributor Author

Hvass-Labs commented Jan 17, 2018

@iaroslav-ai Could you review this? I don't know why Travis fails, I believe the code is working. It's a really small PR that shouldn't take much time to review. It can also be used to simplify a few things in #579 (see the TODO marks in that code).

@Hvass-Labs Hvass-Labs mentioned this pull request Jan 17, 2018
Merged
@holgern
Copy link
Contributor

holgern commented Feb 11, 2020

Is merged in #839
Thanks for your contribution.

@holgern holgern closed this Feb 11, 2020
holgern added a commit that referenced this pull request Feb 11, 2020
@holgern
Merge pull request #839 from holgern/eval_PR_597
2df7746 
[WIP] Eval #597 and add unit test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Hvass-Labs @pep8speaks @holgern