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

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Queue #239

merged 1 commit into from
Aug 15, 2022

Conversation

alonfnt
Copy link
Contributor

@alonfnt alonfnt 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.

@codecov-commenter
Copy link

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
Member

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 =)

@alonfnt
Copy link
Contributor Author

alonfnt 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.

@alonfnt
Copy link
Contributor Author

alonfnt 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. :-)

@codecov-io
Copy link

codecov-io commented Oct 25, 2020

Codecov Report

Merging #239 (310e700) into master (ab8ca64) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   98.44%   98.65%   +0.21%     
==========================================
  Files           7        7              
  Lines         385      372      -13     
  Branches       39       39              
==========================================
- Hits          379      367      -12     
+ 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 ab8ca64...310e700. Read the comment docs.

@alonfnt alonfnt force-pushed the queue branch 2 times, most recently from 847b4ee to 310e700 Compare March 17, 2021 21:02
@alonfnt
Copy link
Contributor Author

alonfnt commented Mar 17, 2021

Fixed!

tests/test_queue.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #239 (8cc8d9b) into master (91441fe) will increase coverage by 0.81%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   98.44%   99.26%   +0.81%     
==========================================
  Files           7        7              
  Lines         387      409      +22     
  Branches       39       58      +19     
==========================================
+ Hits          381      406      +25     
+ Misses          3        1       -2     
+ Partials        3        2       -1     
Impacted Files Coverage Δ
bayes_opt/bayesian_optimization.py 100.00% <100.00%> (+0.96%) ⬆️
bayes_opt/target_space.py 100.00% <0.00%> (ø)
bayes_opt/util.py 97.63% <0.00%> (+0.29%) ⬆️
bayes_opt/domain_reduction.py 100.00% <0.00%> (+4.08%) ⬆️

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 91441fe...8cc8d9b. Read the comment docs.

@bwheelz36
Copy link
Collaborator

Hey @alonfnt, sorry about the slow response. Could I ask you to make any commit to this branch, which will trigger the (recently working again CI) to run?

@alonfnt
Copy link
Contributor Author

alonfnt commented Jul 31, 2022

@bwheelz36 done :)

Previously a custom implementation of Queue was being used with the same
exact functionality as the built-in queue library.
@bwheelz36 bwheelz36 merged commit 2432471 into bayesian-optimization:master Aug 15, 2022
@bwheelz36
Copy link
Collaborator

thank you!

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

6 participants