search

vprusso / toqito

|toqito> (Theory of Quantum Information Toolkit) in Python :snake:
https://toqito.readthedocs.io/en/latest/
MIT License
155 stars 61 forks source link

JOSS review comments #47

Closed marwahaha closed 3 years ago

marwahaha commented 3 years ago

from https://github.com/openjournals/joss-reviews/issues/3082

Overall, neat package. Comprehensive tests. I like the docs page and the tutorials. Few small comments:

I tried the nonlocal game tutorial and received this error when trying to run the nonlocal game. The xor game was ok.

>>> chsh = NonlocalGame(dim, prob_mat, pred_mat)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/kunal/miniconda3/lib/python3.8/site-packages/toqito/nonlocal_games/nonlocal_game.py", line 42, in __init__
    if reps == 1:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Nice work!

vprusso commented 3 years ago

Hi @marwahaha,

Thank you for your outstanding review. Your comments are wonderful, and I really appreciate you taking the time to give this package an honest look and review.

I've addressed your comments in: https://github.com/vprusso/toqito/pull/49.

contributing.md specifies that "multiple core developers will approve PRs", but there seems to be a single core developer.

Amended to specify myself as the core developer.

your codecov check fails often (because the codecov decreases by a tiny fraction like 0.05%) which makes it seems like "the checks don't pass". Could that be relaxed to only fail when codecov decreases by 0.5% or similar?

Added a .codecov.yml file to (hopefully) resolve that issue.

i'm not sure what the proper citation syntax is -- in the paper, consider including a space between the word and the citation, e.g. "physics(citation)" --> "physics (citation)"?

I will err on the side of the latter and make the suggested "physics (citation)" convention.

in the introductory tutorial, the channels and measurements are not well documented : https://toqito.readthedocs.io/en/latest/intro_tutorial.html

There are still quite a few areas to cover, but in the meantime I've placed a "(Coming soon)." marker to indicate that more documentation on a specific subarray will be created there soon.

typo in channel docs: depolarizng -> depolarizing

Nice catch! Fixed.

. How is the randomness generated in toqito.random ?

Depending on the random object being generated, this can vary. The overall randomness comes from the numpy random module. But, for instance, in the case of generated random density matrices, these matrices are generated uniformly at random according to either the Haar or Bures distributions (depending on what the user wants).

what is a perfect matching? the docs claim it takes 2 arguments, but it takes 1 when running the code. i only see outputs for powers of 2.

I attempted to add a bit more to the docs for this function to clear up this point, as it is a good one.

the nonlocal game section has a part that looks like this in the docs

Yet again, a nice catch. Fixed!

are there invalid values for the prob_mat and pred_mat in the nonlocal games? What do the quantum_value and classical_value methods do in this case? (more specifically: how do you know you are using toqito's nonlocal game functionality properly?)

Yes, there are potentially invalid values. Some (basic) checks are in place to make sure that these objects are specified correctly.

I tried the nonlocal game tutorial and received this error when trying to run the nonlocal game. The xor game was ok.

Looks like my tutorial page was not up-to-date with some of the changes I made to the function. I made sure to update that on the tutorials page.

if you think the project is ready, you could 1.0 it, so software users know that they can rely on a consistent interface. I guess this is controversial, see here and here, but I would recommend a 1.0.0 release so additional changes are explicit to current users.

That's a good point. I took you up on your advice and went for 1.0.0. Thanks for the suggestion!

vprusso commented 3 years ago

Hi @marwahaha. Apologies if I'm interpreting the review process incorrectly, but do I provide my responses and adaptions via a pull request, as I did, and then merge the PR assuming the comments and response are satisfactory? I wanted to ensure I've addressed your comments prior to moving onto the second review. Thanks again for all of your helpful feedback!

marwahaha commented 3 years ago

Hi @vprusso ! I don't know what the "best process" is. I did a 30-second glance at the PR and it looks ok, but I'm on vacation and won't be able to really look until next week. I'm ok with you merging it so you can move on to the next review, and I can take another glance next week. Does that work for you?

vprusso commented 3 years ago

That sounds great. My apologies for interrupting you during your vacation!

marwahaha commented 3 years ago

A few quick comments:

Nice work!

marwahaha commented 3 years ago

You may also consider giving a talk here: https://unitary.fund/talks.html

vprusso commented 3 years ago

Hi @marwahaha

contributing.md: I think it should be "approves the reviews" vs "will approve the reviews"

Agreed, I've gone ahead and made this change.

perfect matchings # of arguments -- maybe it's an issue with the readthedocs generation, but it doesn't look like it's reading the signature correctly:

Ah, this actually appeared to be some type of subtle problem with autosummary (https://github.com/sphinx-doc/sphinx/issues/6311). The fix for me was to apply the feature created in (https://github.com/sphinx-doc/sphinx/issues/6361) and add the line autodoc_typehints = 'none' to the conf.py file responsible for generating the Sphinx docs. After regenerating with that option, it seems to fix the issue. Nice catch!

nonlocal games -- I think the tutorial should say chsh.quantum_value_lower_bound() not chsh.quantum_lower_bound()

For this one, I may suggest sticking to quantum_value_lower_bound() over quantum_lower_bound(). The reason for this is consistency in naming with the other values for nonlocal games--for instance nonsignaling_value and classical_value. You can let me know if you agree or disagree with this naming logic.

I think the FFL game tutorial has the same issue. Also, the number doesn't match up (I get 0.2222 not 0.6666).

Nice catch. Thankfully this was just an issue with the tutorial example and not with the code itself. I made sure to test the example and update it as well. Again, great catch!

Make sure you push the 1.0 to pypi when you are ready.

Awesome, done! :) : https://pypi.org/project/toqito/

You may also consider giving a talk here: https://unitary.fund/talks.html

I would love to! I've done some prior work with the UnitaryFund and look forward to continuing to do so! Thanks for posting that and keeping the toqito project in mind!

marwahaha commented 3 years ago

I think the naming logic is fine. Ctrl-F on https://toqito.readthedocs.io/en/latest/tutorials.nonlocal_games.html for quantum_lower_bound and I think it should instead be quantum_value_lower_bound.

vprusso commented 3 years ago

@marwahaha. Wonderful. Nice catch, and done! :)