-
Notifications
You must be signed in to change notification settings - Fork 95
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
base: main
Are you sure you want to change the base?
Conversation
@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 |
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 :) |
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 :) |
No problem at all, take your time! |
Hi @MischaPanch 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 |
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 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 :) |
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 :) |
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