search

bayesian-optimization / BayesianOptimization

A Python implementation of global optimization with gaussian processes.
https://bayesian-optimization.github.io/BayesianOptimization/index.html
MIT License
7.81k stars 1.53k forks source link

Potential for ANSI colours to remain active #488

Closed d7919 closed 2 months ago

d7919 commented 2 months ago

In _step of logger, the colour remains active after the line is displayed, i.e. it is not reset.

https://github.com/bayesian-optimization/BayesianOptimization/blob/3a5e56c7d0f96affdebfebc318b02f71f0462be3/bayes_opt/logger.py#L166

If the user's function has any output or if the system displays any output this will inherit the colour of the last output. This can also potentially cause issues if the user kills the job mid-run leaving the colour code active.

Changing this line to 

return "| " + " | ".join([colour + cells[i] + Fore.RESET for i in range(len(cells))]) + " |"

is perhaps a bit "safer" and has the minor advantage of not colouring the | column separators.

On a side note, it would be helpful if the user could:

  1. Turn off the colouring (if capturing output the escape codes can enter the log file).
  2. Choose the two colours in use -- for me the default Black is unreadable due to the colour of my terminal background.
till-m commented 2 months ago

Hi @d7919,

regarding the cause for the issue, good catch, this should indeed be fixed. Thanks for providing a PR!

side note

you raise some good points:

Choose the two colours in use -- for me the default Black is unreadable due to the colour of my terminal background.

I think it would make sense to replace Fore.BLACK with Fore.RESET in general. What do you think? This would probably alleviate the issue for most cases and be relatively low effort. However, if you feel strongly about making the ScreenLogger more flexible/customizable (turning colours off or using different colours), and are willing submit a PR for this, I will gladly review and merge it :)

till-m commented 2 months ago

I will close this for now