TaipanRex / pyvisgraph

Given a list of simple obstacle polygons, build the visibility graph and find the shortest path between two points
MIT License
217 stars 45 forks source link

Feature/bugfix: changed progressbar to tqdm #32

Closed tjansson60 closed 6 years ago

tjansson60 commented 6 years ago

The existing statusbar did not work properly with workers>1. This patch changes the progressbar to use tqdm. This means that the new progressbar works nicely with 1 or many workers and in addition comes with:

TaipanRex commented 6 years ago

I am always careful with adding dependencies, but reading about tqdm it seems to have a very small overhead and works with windows and linux (which my hacky progress bar does not).

Let me try out your patch first, but looks good!

tjansson60 commented 6 years ago

Very understandable and I you have another progressbar package you would rather use I can easily can rewrite the patch. The reason chose tqdm is that I think it is quite mature and quite fast compared to progressbar according to their docs.

TaipanRex commented 6 years ago

Yeah I just tested it for a while and at least in linux I am getting no performance degradation. I was thinking at first that the smaller batch sizes would hurt performance, but it doesn't because with larger batch sizes, usually one batch will be slower than the rest. This might even be faster in some cases.

We will need to refactor out the status parameter to the build() method, its not used anymore. I think its better to just have the status bar on at all times.

Great job, it looks a lot better this way.

tjansson60 commented 6 years ago

Do you want to just remove it now completely or leave a print to screen if anybody set it to false and say it is deprecated? I am unaware of how many users there are of the project there are and if backwards compatibility is important or not.

TaipanRex commented 6 years ago

I followed your suggestion and deprecated it instead. merged & released.

tjansson60 commented 6 years ago

Great! 👍