X Tutup
The Wayback Machine - https://web.archive.org/web/20201111110723/https://github.com/fmfn/BayesianOptimization/pull/239
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

Queue #239

Open
wants to merge 3 commits into
base: master
from
Open

Queue #239

wants to merge 3 commits into from

Conversation

@AlbertAlonso
Copy link
Contributor

@AlbertAlonso AlbertAlonso commented Jul 7, 2020

Browsing the code I've seen a custom implementation of Queue. Since there is a queue on the Standard Library, why not use it? it may be useful when doing threading? and there is less code to maintain.

If this is not the direction you want to take feel free to close this PR. I just did it because it seems like reinventing the wheel a bit.

Rebase
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jul 7, 2020

Codecov Report

Merging #239 into master will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   98.44%   98.64%   +0.20%     
==========================================
  Files           7        7              
  Lines         385      369      -16     
  Branches       39       39              
==========================================
- Hits          379      364      -15     
+ Misses          3        2       -1     
  Partials        3        3              
Impacted Files Coverage Δ
bayes_opt/bayesian_optimization.py 100.00% <100.00%> (+0.98%) ⬆️

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 e32b1fb...59b976d. Read the comment docs.

@fmfn
Copy link
Owner

@fmfn fmfn commented Jul 8, 2020

Of course there is 🤦 .
When implementing it I didn't even think about it, but I agree, not re-inventing the wheel is definitely the way to go. If you don't mind fixing the conflict and squashing the commits into 1, max 2, commits, I would love to get this merged =)

@AlbertAlonso AlbertAlonso force-pushed the AlbertAlonso:queue branch from 4c8b714 to a081900 Jul 9, 2020
@AlbertAlonso
Copy link
Contributor Author

@AlbertAlonso AlbertAlonso commented Jul 9, 2020

Ok, first time squashing and it seems I have squashed more that what I should. I will have to check how to fix it, my bad.

@AlbertAlonso
Copy link
Contributor Author

@AlbertAlonso AlbertAlonso commented Jul 29, 2020

Actually, the Files Changed section of the PR seems about right, but if I check the use queue from the standard library commit, it seems to be including some documentation and some other commits. I haven't been able to "fix" it, so maybe it is already ok (?).
Please let me know what you think. :-)

@AlbertAlonso AlbertAlonso force-pushed the AlbertAlonso:queue branch from 59b976d to 847b4ee Oct 25, 2020
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 25, 2020

Codecov Report

Merging #239 into master will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   98.44%   98.65%   +0.20%     
==========================================
  Files           7        7              
  Lines         387      371      -16     
  Branches       39       39              
==========================================
- Hits          381      366      -15     
+ Misses          3        2       -1     
  Partials        3        3              
Impacted Files Coverage Δ
bayes_opt/bayesian_optimization.py 100.00% <100.00%> (+0.96%) ⬆️

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 380b0d5...6819e5f. Read the comment docs.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.
X Tutup