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

Add option to show single progress bar for all workers #239

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

lpuglia
Copy link

@lpuglia lpuglia commented May 23, 2023

Not sure why ApolloBian didn't reply further to the PR(#203) but i'm here to make sure this change is merged.

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #239 (545ac80) into master (46fc0e5) will decrease coverage by 0.83%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
- Coverage   91.37%   90.55%   -0.83%     
==========================================
  Files          12       12              
  Lines         580      593      +13     
==========================================
+ Hits          530      537       +7     
- Misses         50       56       +6     
Impacted Files Coverage Δ
pandarallel/progress_bars.py 72.93% <53.84%> (-2.48%) ⬇️
pandarallel/core.py 91.09% <100.00%> (+0.09%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@till-m
Copy link
Collaborator

till-m commented May 23, 2023

Hi @lpuglia, thanks for taking care of this. Could you also make sure this change is covered by adding a fixture for the single bar option (see e.g. this)?

pandarallel/core.py Outdated Show resolved Hide resolved
@lpuglia
Copy link
Author

lpuglia commented May 23, 2023

@till-m should I add tests for codecov?

@till-m
Copy link
Collaborator

till-m commented May 23, 2023

The uncovered lines seem to only relate to the use in notebooks, I don't know any convenient way to test them. If you have something, feel free to let me know. I will try to test them manually by tomorrow so that I can merge this with no further delay.

@till-m till-m requested review from till-m and nalepae May 23, 2023 11:26
Copy link
Collaborator

@till-m till-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lpuglia,

I tested it, and it works, so the PR looks good in general to me.

Two small changes:
I think the code should either use single_bar or single_progress_bar, not both. And I would also ask you to briefly document the functionality in the user guide.

@lpuglia
Copy link
Author

lpuglia commented Jun 22, 2023

@till-m done.

By the way, it looks fine in both ipywidget
image
and text mode:
image

@till-m
Copy link
Collaborator

till-m commented Jun 26, 2023

Hi @lpuglia,

thanks for changing the function argument name. Could you also add a small line in the documentation, specifically here updating the function parameter description?

@@ -470,6 +474,7 @@ def initialize(
if use_memory_fs
else parallelize_with_pipe
)
parallelize = partial(parallelize, single_bar=single_bar)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO instead it would be better to supply the argument in lines 528 onward as it is currently done with the arguments of parallelize. @nalepae any opinion?

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

3 participants