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

Fix SKOptLearner for multi variate domain (issue #233) #234

Merged
merged 5 commits into from
Dec 8, 2019
Merged

Fix SKOptLearner for multi variate domain (issue #233) #234

merged 5 commits into from
Dec 8, 2019

Conversation

caenrigen
Copy link
Contributor

@caenrigen caenrigen commented Dec 5, 2019

This fixes issue #233

Tests checked. Per-commit checked.
Implemented additional test for this case.

Changed data type to OrderedDict to comply with other learners.

@caenrigen
Copy link
Contributor Author

caenrigen commented Dec 5, 2019

Strange that the linting_test failed
I get no error when running locally pre-commit run --all-files

How can it be fixed?

@jbweston
Copy link
Contributor

jbweston commented Dec 5, 2019

Strange that the linting_test failed
I get no error when running locally pre-commit run --all-files

How can it be fixed?

When I click through and look at the test failure I see that it's because black reformatted some files.

I am not sure why pre-commit succeeds on your machine. When I check out your branch locally and run pre-commit run --all-files the black check indeed fails.

Copy link
Contributor

@jbweston jbweston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM module formatting changes

@jbweston jbweston requested a review from basnijholt December 5, 2019 15:11
@caenrigen
Copy link
Contributor Author

caenrigen commented Dec 5, 2019

Strange that the linting_test failed
I get no error when running locally pre-commit run --all-files
How can it be fixed?

When I click through and look at the test failure I see that it's because black reformatted some files.

I am not sure why pre-commit succeeds on your machine. When I check out your branch locally and run pre-commit run --all-files the black check indeed fails.

You are right there was some message about black the first time I run pre-commit, I didn't notice it, I see the formatting changed in the file and it doesn't complain anymore.
[Update] Fixed

Copy link
Member

@basnijholt basnijholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot for your contribution!

@basnijholt
Copy link
Member

@jbweston, should we just merge this into master? Seems like we didn't put anything on https://github.com/python-adaptive/adaptive/tree/stable-0.9.

@caenrigen
Copy link
Contributor Author

LGTM! Thanks a lot for your contribution!

Thank you, guys, for the nice package! 😃

@jbweston
Copy link
Contributor

jbweston commented Dec 6, 2019

@jbweston, should we just merge this into master? Seems like we didn't put anything on https://github.com/python-adaptive/adaptive/tree/stable-0.9.

Depends; do we want to release v0.9.1? So far we have not been properly adhering to semver, which I think is fine in our pre-1.0 no-backwards-compatibility state. I think we can elide keeping stable-* branches around always put things into the next minor release.

@basnijholt
Copy link
Member

I think we can skip a 0.9.1. So let's merge!

@jbweston jbweston merged commit 2c95367 into python-adaptive:master Dec 8, 2019
@basnijholt basnijholt mentioned this pull request Jan 16, 2020
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.

3 participants