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

Move html progressbar #2112

Merged
merged 5 commits into from Mar 9, 2023
Merged

Conversation

HarshKhilawala
Copy link
Contributor

@HarshKhilawala HarshKhilawala commented Mar 4, 2023

Checklist
Thank you for contributing to QuTiP! Please make sure you have finished the following tasks before opening the PR.

  • Please read Contributing to QuTiP Development
  • Contributions to qutip should follow the pep8 style.
    You can use pycodestyle to check your code automatically
  • Please add tests to cover your changes if applicable.
  • If the behavior of the code has changed or new feature has been added, please also update the documentation in the doc folder, and the notebook. Feel free to ask if you are not sure.
  • Include the changelog in a file named: doc/changes/<PR number>.<type> 'type' can be one of the following: feature, bugfix, doc, removal, misc, or deprecation (see here for more information).

Delete this checklist after you have completed all the tasks. If you have not finished them all, you can also open a Draft Pull Request to let the others know this on-going work and keep this checklist in the PR description.

Description
Moved HTMLProgressBar from qutip/ipynbtools.py to qutip/ui/progressbar.py

Related issues or PRs
Fix #2108

@HarshKhilawala
Copy link
Contributor Author

@Ericgig Please review the PR.Thanks!

@HarshKhilawala
Copy link
Contributor Author

@AGaliciaMartinez Can you please review this PR and get it merged?

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

The change itself looks good. I left one small suggestion. I've also activated the test runs, so we should see if those pass.

It would be good to add tests for the progress bars, but perhaps that is best done in a follow up PR. For example, I know that the TqdmProgressBar currently does not work.

qutip/ui/progressbar.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 8, 2023

Coverage Status

Coverage: 72.089%. Remained the same when pulling 8de851f on HarshKhilawala:move-html-progressbar into c499d44 on qutip:master.

@HarshKhilawala
Copy link
Contributor Author

@hodgestar PTAL! Thanks

@hodgestar hodgestar merged commit 40ff8e1 into qutip:master Mar 9, 2023
@hodgestar
Copy link
Contributor

Thanks @HarshKhilawala!

@hodgestar
Copy link
Contributor

@HarshKhilawala Would you be interested in adding some tests for the progress bars in another PR?

@HarshKhilawala
Copy link
Contributor Author

@hodgestar Sure, do I need to be assigned another issue? Can I create new issue for writing tests for progress bar and get assigned to work on it?

@hodgestar
Copy link
Contributor

hodgestar commented Mar 9, 2023 via email

@HarshKhilawala
Copy link
Contributor Author

@hodgestar I am not much aware with writing tests. Can you help me write test for progressbar file? How to write a test? How to check coverage for the entire file? How to check coverage for each line of code as to know where and what to add test? From where can I learn more about testing? Any documentation for writing tests specifically for qutip repository?

@HarshKhilawala
Copy link
Contributor Author

@hodgestar Anything for reference? Any pre-existing test_code which I can use to write tests for progressbar? PTAL!

@HarshKhilawala
Copy link
Contributor Author

@Ericgig Can you help me with this one? I am considering adding tests for progressbar.

@Ericgig
Copy link
Member

Ericgig commented Mar 16, 2023

There are no test code for progress bars yet.
You will need to create a new test file: qutip/tests/test_progressbar.py.

The test can be as simple as creating the bar, call update, call finished and check that the total time is a sensible value.
You could also use pytest to catch print with capsys and confirm that something was printed when update / finished is called.

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.

Allow solvers to use HTMLProgressBar
4 participants