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

MAINT: replace progressbar with tqdm #202

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Conversation

fkuehlein
Copy link
Collaborator

Deleted the utils/progressbar and everything that relates to it. Added tqdm dependency and replaced all ProgressBar instances with tqdm progress bars.

@ntfrgl, I'm not entirely sure I added the tqdm dependency in all the right places in setup.cfg and .travis.yml, could you please double check?

@fkuehlein fkuehlein added this to the New Release milestone Oct 12, 2023
@fkuehlein fkuehlein linked an issue Oct 12, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #202 (585c503) into master (ea721c1) will decrease coverage by 0.19%.
The diff coverage is 22.72%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   52.04%   51.85%   -0.19%     
==========================================
  Files          49       45       -4     
  Lines        7473     7064     -409     
==========================================
- Hits         3889     3663     -226     
+ Misses       3584     3401     -183     
Files Coverage Δ
src/pyunicorn/climate/havlin.py 61.36% <100.00%> (-2.85%) ⬇️
src/pyunicorn/climate/mutual_info.py 71.25% <ø> (+19.49%) ⬆️
src/pyunicorn/timeseries/surrogates.py 51.89% <50.00%> (+1.63%) ⬆️
src/pyunicorn/core/network.py 54.64% <11.11%> (+<0.01%) ⬆️

@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Oct 12, 2023

CodeCov is complaining that overall test coverage is actually slightly reduced. Apparently, the few additional lines in untested methods outweigh the countless untested lines I removed. Not sure I understand the logic, could add tests of course, but I'd say that's a different issue.

if (i % 10) == 0:
progress.update(i)
# toggle progress bar
silence = self.silence_level > 1
Copy link
Member

Choose a reason for hiding this comment

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

I would inline this into the trange() call.

Copy link
Member

Choose a reason for hiding this comment

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

This obviously applies equally to the other changes.

@ntfrgl
Copy link
Member

ntfrgl commented Oct 13, 2023

This coverage metric ended up slightly lower, because the coverage within the old utils/progressbar, which was entered via the removed lines in the tested methods, was somewhat higher than the current overall average over pyunicorn. Irrespectively of the particular metric, this PR is an improvement to the project code base.

For the record, the following are the untested methods in which the progress bar was replaced, and which appear not to be used anywhere else in the project. The last one in particular has been replaced by its Cythonized version, so I suggest deleting it.

I would also suggest to start writing the change log for the new release. This PR would be represented by a one-liner there.

Otherwise, this looks good to me.

- inline `silence_level` condition into `trange()` call
- as was unused, in favour of `_cython_calculate_mutual_information()`
- lint fix to `104ca9e`
@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Oct 13, 2023

Thanks for the hints!

I improved the silencing of the new progressbars accordingly and removed MutualInfoClimateNetwork._calculate_mutual_information‎(). Made CodeCov slightly happier, too!

Wouldn't want to decide about the other apparently stale methods myself. Maybe @jdonges can have a quick look? Shouldn't keep us from merging this though.

Oh and I started taking notes for the changelog now, good point.

@ntfrgl
Copy link
Member

ntfrgl commented Oct 13, 2023

Thank you.

In contrast to MutualInfoClimateNetwork._calculate_mutual_information‎(), the other untested methods are public, so I don't see a reason for removing them at this point.

@ntfrgl ntfrgl merged commit 29c27f0 into master Oct 13, 2023
1 of 3 checks passed
@fkuehlein fkuehlein deleted the 197-replace-progressbar-tqdm branch October 23, 2023 10:58
@ntfrgl ntfrgl added the maintenance something should be improved or is outdated label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance something should be improved or is outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace: progressbar -> tqdm
2 participants