Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added TypedConfigurationSpace #345

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MischaPanch
Copy link

@MischaPanch MischaPanch commented Dec 11, 2023

I provided two ways of creating typed configuration spaces - one binding a class to an output type, and another where the type constructor is simply passed at init. The second option requires less code, doesn't use generics, and many users might prefer it. However, the first is more explicit, allows for better discoverability and might be prefered by users who have to deal with mutliple config spaces

I have also fixed some typos and slightly enhanced input validation. Previously, passing name={..}, space=some_space would have simply ignored that space was passed at all

@MischaPanch
Copy link
Author

@eddiebergman Here the PR - better late than never :). Would be happy if it could be merged in the foreseeable future, as I'd like to use configspace for our projects. Meanwhile, I can of course just use this extension in our own codebase

@eddiebergman
Copy link
Contributor

Hi @MischaPanch, I won't be able to properly review it until I'm back next week but I've set a reminder for it! Many thanks for the PR :)

@eddiebergman
Copy link
Contributor

Also as a side note, there'll likely be build issues due to needing to update the types to comply with the newer version of CYthon, which will likely take a few days to complete as well, before we can make it publically available on pypi. It's some technical debt I've been ignoring for a while but it's a good reason to update it :)

@eddiebergman eddiebergman self-assigned this Dec 12, 2023
@MischaPanch
Copy link
Author

Hi @MischaPanch, I won't be able to properly review it until I'm back next week but I've set a reminder for it! Many thanks for the PR :)

No problem at all, take your time!

@eddiebergman
Copy link
Contributor

eddiebergman commented Dec 18, 2023

Hi @MischaPanch
There is an issue in that using Cython 3 to compile ConfigSpace doesn't work anymore as it now expects more rigid typing (of which there are quite a few inconsistencies).

You might need to rebase once #346 is done, from what I checked, the only thing you'll need to actively fix is then the unittest test case for what you have. I will do a review once the refactor is done anywho

@eddiebergman
Copy link
Contributor

Hi @MischaPanch,

It's taken a while but there was a major refactor in #346 which means this PR would likely need to be updated. I'm also happy to go ahead and try implementing the feature in the refactored version given this PR as a base and inspiration!

FYI: I've tested and everythings been deprecated properly such that most old ConfigSpace tests and all SMAC's unittests pass with #346 installed. However you might want to know that #346 hard removed "q" quantization of hyperparameters and unbounded hyperparameters, everything else is soft-deprecated and should function as before.

I think this should be closed and a new PR re-opened, but I will leave it to you if you'd like to continue this work!

Also, sorry for the delay on this but hopefully you like the new ConfigSpace :)

@MischaPanch
Copy link
Author

Cool, congrats on the refactoring! I can rebase on the newest version or submit a separate PR at the end of next week. Looking forward to trying out the new configspace :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants