toqito icon indicating copy to clipboard operation
toqito copied to clipboard

PyCVXQuad 10/21

Open simsaidan opened this issue 1 year ago • 3 comments

Description

Provide a brief description of the PR's purpose here. If your PR is supposed to fix an existing issue, use a keyword to link your PR to the issue.

Changes

Notable changes that this PR has either accomplished or will accomplish. Feel free to add more lines to the itemized list below.

  • [ ] Change 1

Checklist

Before marking your PR ready for review, make sure you checked the following locally. If this is your first PR, you might be notified of some workflow failures after a maintainer has approved the workflow jobs to be run on your PR.

Additional information is available in the documentation.

  • [ ] Use ruff for errors related to code style and formatting.
  • [ ] Verify all previous and newly added unit tests pass in pytest.
  • [ ] Check the documentation build does not lead to any failures. Sphinx build can be checked locally for any failures related to your PR
  • [ ] Use linkcheck to check for broken links in the documentation
  • [ ] Use doctest to verify the examples in the function docstrings work as expected.

simsaidan avatar Oct 21 '24 19:10 simsaidan

@simsaidan just FYI, I fixed the one merge conflict that was preventing the subsequent pipelines from running. We should now expect to see some linting failures, but these should be easy enough to resolve. Of course, don't hesitate to reach out if you'd like to ask any questions or if you want to triage some of these together. Let me know what makes sense for you!

vprusso avatar Oct 21 '24 21:10 vprusso

Hi @simsaidan, the docstring template in toqito's contributing guide is not up to date. I noticed it during your mid-point check-in presentation. My apologies!

I am linking an example function for what it is supposed to look like:

https://github.com/vprusso/toqito/blob/3b91eb68b19729f9a43192556e16a5a4b0b1fdc6/toqito/channel_props/is_unital.py#L58-L62

You have to use :param name_of_parameter: Description of parameter for it to show up formatted correctly in the RTD build. We can make an issue for correcting the docstrings of the functions already in this PR. Just thought you should know for the remaining functions.

If you want to take a look at how the docstrings are formatted in the docs build, make sure you add your cvx_quad module to docs/autoapi_members.rst.

Edit: The docstring template should be corrected now.

purva-thakre avatar Oct 23 '24 16:10 purva-thakre

Hi @simsaidan, Can you please make title and summary of the PR more descriptive? It helps potential reviewers that are not familiar with your project.

Also my advice would be to first make a smaller PR with a single quantity (+test and auxiliary functions, if needed) where we can agree on things like styles, module organization, test approach, dependencies.

cosenal avatar Oct 24 '24 08:10 cosenal

@simsaidan are we closing this MR in favor of more direct integration with https://arxiv.org/abs/2410.17803?

vprusso avatar Mar 08 '25 17:03 vprusso

@vprusso yes, we can close this MR

simsaidan avatar Mar 10 '25 16:03 simsaidan