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

feat: added random_states func + tests #383

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Conversation

ankit-pn
Copy link
Contributor

@ankit-pn ankit-pn commented Dec 19, 2023

Description

added random_states function and its test as described in issue #380

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Implemented random_state function in rand/random_states.py
  • Implemented its test in rand/test/test_random_states.py

Status

  • Ready to go

@ankit-pn
Copy link
Contributor Author

ankit-pn commented Dec 19, 2023

Is there any doc specifying what kind of code style that we need to follow, or which formatter I need to use to format the python code?

@purva-thakre
Copy link
Collaborator

purva-thakre commented Dec 19, 2023

@ankit-pn if you look at the output of the failing tests, it tells you what error is being caught by which formatter/linter.

Once those are fixed, you will also have to add qiskit as a dependency in the following file otherwise check-build workflow will fail because qiskit can't be imported by pytest.

[tool.poetry.dependencies]

Included qiskit in the [tool.poetry.dependencies] section to enable quantum computing functionalities required for the toqito project.
@ankit-pn
Copy link
Contributor Author

@ankit-pn if you look at the output of the failing tests, it tells you what error is being caught by which formatter/linter.

Once those are fixed, you will also have to add qiskit as a dependency in the following file otherwise check-build workflow will fail because qiskit can't be imported by pytest.

[tool.poetry.dependencies]

Thanks Purva, I have added qiskit as dependencies in pyproject.toml and formatted the code accordingly.

@vprusso
Copy link
Owner

vprusso commented Dec 20, 2023

Also, don't forget to address the issues that the linter is catching here

Additionally, you'll need to add toqito.rand.random_state to docs/states.rst under the "States" heading.

Feel free to ping if you have any questions!

@purva-thakre
Copy link
Collaborator

@ankit-pn To check if all the workflows pass locally (after all the dependencies are installed in your virtual environment)

  • For ruff, use ruff check . in the main toqito directory.
  • For pylint, use poetry run pylint --fail-under=10.0 toqito/ in the main toqito directory
  • For pytest use pytest in toqito/toqito

If you don't understand what an error is trying to tell you, you can look at how the function docstrings/tests are formatted in other modules.

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (73641d8) 97.5% compared to head (38706f6) 97.6%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #383   +/-   ##
======================================
  Coverage    97.5%   97.6%           
======================================
  Files         152     153    +1     
  Lines        3043    3042    -1     
  Branches      743     742    -1     
======================================
+ Hits         2968    2970    +2     
+ Misses         46      45    -1     
+ Partials       29      27    -2     

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

Comment on lines 20 to 31
Parameters
----------
n : int
The number of random states to generate.
d : int
The dimension of each quantum state.

Returns
-------
list[numpy.ndarray]
A list of `n` numpy arrays. Each array is a d-dimensional quantum state represented
as a column vector.
Copy link
Collaborator

@purva-thakre purva-thakre Dec 20, 2023

Choose a reason for hiding this comment

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

You do want to make sure you stay consistent with how these are listed in the docstrings of existing functions.

For example,

:raises ValueError: If matrix is not Choi matrix.
:param mat: A matrix.
:param phi_op: A superoperator. :code:`phi_op` should be provided either as a Choi matrix,
or as a list of numpy arrays with either 1 or 2 columns whose entries are its
Kraus operators.
:return: The result of applying the superoperator :code:`phi_op` to the operator :code:`mat`.

Also, make sure you add this new function to the docs as Vincent said in his comment.

Copy link
Contributor Author

@ankit-pn ankit-pn Dec 20, 2023

Choose a reason for hiding this comment

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

specifying parameter with :param tag is failing ruff check . . is there any way to bypass it. I am getting this error if i remove argument description that i have included in previous pr and use :param tag.

ruff check rand/ --fix
rand/random_states.py:7:5: D417 Missing argument descriptions in the docstring for `random_states`: `d`, `n`
Found 1 error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is your docstring following everything as expected? For example, Ruff is set to using === for section headings instead of ---.

Pay attention to the full docstring I linked above and don't use an auto-formatter. FYI, we don't use black.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have pushed a change, pls review it and let me if it is ok or i need to make some more change.

@vprusso
Copy link
Owner

vprusso commented Dec 20, 2023

Also, don't forget to address the issues that the linter is catching here

Additionally, you'll need to add toqito.rand.random_state to docs/states.rst under the "States" heading.

Feel free to ping if you have any questions!

Seems like this still isn't part of the diff, @ankit-pn

@ankit-pn
Copy link
Contributor Author

It should be added to rand.rst?

@ankit-pn
Copy link
Contributor Author

has made fixes for it, can you check? have added toqito.rand.random_state to docs/rand.rst.

@vprusso
Copy link
Owner

vprusso commented Dec 20, 2023

has made fixes for it, can you check? have added toqito.rand.random_state to docs/rand.rst.

Yep! That's what I was suggesting. Nice!

Copy link
Owner

@vprusso vprusso left a comment

Choose a reason for hiding this comment

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

LGTM! I'll approve but want to wait for @purva-thakre to give the final sign-off.

Great work, @ankit-pn, thank you again for your contribution!

@purva-thakre
Copy link
Collaborator

LGTM too! 🎆

@vprusso vprusso merged commit 201eb03 into vprusso:master Dec 21, 2023
10 checks passed
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.

3 participants