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

Removing redundant note for doctest. #651

Merged
merged 12 commits into from
Jul 10, 2024
Merged

Removing redundant note for doctest. #651

merged 12 commits into from
Jul 10, 2024

Conversation

vprusso
Copy link
Owner

@vprusso vprusso commented Jul 7, 2024

Closes #650

@vprusso vprusso requested a review from purva-thakre July 7, 2024 13:19
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.7%. Comparing base (372c205) to head (c3111f3).
Report is 2 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #651   +/-   ##
======================================
  Coverage    97.7%   97.7%           
======================================
  Files         166     166           
  Lines        3250    3250           
  Branches      795     795           
======================================
  Hits         3176    3176           
  Misses         48      48           
  Partials       26      26           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@purva-thakre
Copy link
Collaborator

purva-thakre commented Jul 7, 2024

It appears all over the place, and I find this a bit distracting (especially when noted numerous times in the same tutorial, etc.)

I am fine with removing this if it is repeated across one page.

We should mention it once for the other pages. The things required by DOCTEST don't make sense for someone who is not familiar with why we are using something in the print output. What if they copy-paste an example into a local jupyter notebook? I had added it for this reason.

@vprusso
Copy link
Owner Author

vprusso commented Jul 8, 2024

It appears all over the place, and I find this a bit distracting (especially when noted numerous times in the same tutorial, etc.)

I am fine with removing this if it is repeated across one page.

We should mention it once for the other pages. The things required by DOCTEST don't make sense for someone who is not familiar with why we are using something in the print output. What if they copy-paste an example into a local jupyter notebook? I had added it for this reason.

That makes sense--I think having it at least in one place definitely makes sense. Maybe in the contribution guide?

@purva-thakre
Copy link
Collaborator

I think having it at least in one place definitely makes sense. Maybe in the contribution guide?

What if we create a section called Examples in the Getting Started section? We could introduce all the tutorials, point a user to the examples in the API as well as have a note for the things we use for DOCTEST.

Adding it to the contribution guide wouldn't be helpful for someone who is not interested in contributing to the project. Thoughts?

Comment on lines 43 to 44
>>> np.around(entanglement_of_formation(rho), decimals=2)
1.00
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>>> np.around(entanglement_of_formation(rho), decimals=2)
1.00
>>> import numpy as np
>>> np.around(entanglement_of_formation(rho), decimals=2)
1.00

Sometimes np.around will round up to a higher number when we don't want it to. We don't see that here but when a function relies on optimization, np.around(0.8) gets rounded up to 1.0. This is why I did not use it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, but this would only occur if you don't specify the number of decimals.

So, for instance:

np.around(0.66)
1.0

However

np.around(0.66, decimals=2)
0.66

@vprusso vprusso merged commit c77ec5c into master Jul 10, 2024
18 checks passed
@vprusso vprusso deleted the 650-remove-float-note branch July 10, 2024 17:33
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.

Remove note on float for documentation
2 participants