Issues/394

[LemurPwned]

Hey,
here's my suggestion to fix the trimming issue. The tests seem to pass, and the global bounds should be respected.

A suggestion perhaps to add contribution guideline (formatting etc.).
Sorry for the initial erroneous commit -- please squash if it's ok.

[till-m]

Hi @LemurPwned,

thanks for your contribution.
I think this generally looks good. Two things that would improve this PR imo are additional tests that cover the specific case of #394 & checking in the _trim() function whether minimum_window and the global bounds are actually compatible, and raise ValueError when they aren't.

I'm not a maintainer on this project though, so this is just a personal opinion.

[LemurPwned]

Thanks @till-m -- I think these are very sensible suggestions, I'll try to implement them soon, when I get some spare time.

[bwheelz36]

@till-m of course you are a maintainer! you are doing more than anyone to keep this project maintained!

[bwheelz36]

@LemurPwned don't worry about the tiny codecov failure, if you implement the planned changes happy to accept

[LemurPwned]

@till-m

• I added the _window_bounds_compatiblity which checks whether global bounds are compatible with window sizes at the initialisation stage. The _trim function already operates in such a way that it should not infringe on the global bounds - window sizes compatibility, so I suspect checking it once at the start should work. Does this make sense?
• Added two tests. test_trimming_bounds checks the sets of use cases similar to the Sequential Domain Reduction does not respect global bounds #394 -- it does a functional test, I did not want to engineer a function to meet the case. test_exceeded_bounds is a test where the user has given a set of bounds such that they can't be reconciled with the minimum_window. Assertion is on ValueError raised.

Let me know if that works.

Thanks again @till-m and @bwheelz36 for looking at this.

[till-m]

Thanks, this looks good to me 👌

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.52 ⚠️

Comparison is base (202dc3c) 99.08% compared to head (ea1a95e) 98.57%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
- Coverage   99.08%   98.57%   -0.52%     
==========================================
  Files           8        8              
  Lines         548      560      +12     
  Branches       78       79       +1     
==========================================
+ Hits          543      552       +9     
- Misses          2        4       +2     
- Partials        3        4       +1     

Impacted Files	Coverage Δ
bayes_opt/domain_reduction.py	96.00% <100.00%> (-4.00%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.