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

Remove vendored qcircuit.tex #61

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

jakelishman
Copy link
Member

This file included GPL'd code, which is incompatible with QuTiP's 3-clause BSD licence. Instead, we rely on the user to have a functional install of the file (it's been included in TeXLive since at least 2014).

This also attempts to maintain patches made to the LaTeX code.

See qutip/qutip#1579

This is the same PR as qutip/qutip#1580, effectively.

This file included GPL'd code, which is incompatible with QuTiP's
3-clause BSD licence.  Instead, we rely on the user to have a functional
install of the file (it's been included in TeXLive since at least 2014).

This also attempts to maintain patches made to the LaTeX code.

See qutip/qutip#1579
@jakelishman jakelishman requested a review from BoxiLi June 18, 2021 15:16
Comment on lines +238 to +239
" Perhaps you do not have it installed, or you are"
" missing the LaTeX package 'qcircuit'."
Copy link
Contributor

Choose a reason for hiding this comment

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

@jakelishman Thanks for this ! qcircuit not installed was one of the issues I was facing in #60.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just having a flick through #60, it looks like you might be looking for too much on conda - imagemagick is something you'd typically expect to find in a system package manager, rather than conda. On Debian-like Linuxes that would be likely be apt (apt[-get] install imagemagick), or brew install imagemagick on Mac.

qcircuit itself should be present in any fully featured TeXLive installation. If you've just got a minimal TeXLive it might not have it, but you can use tlmgr to install it.

Copy link
Contributor

@purva-thakre purva-thakre Jun 18, 2021

Choose a reason for hiding this comment

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

qcircuit itself should be present in any fully featured TeXLive installation

I had a minimal install with qcircuit but not all of it's dependencies were installed. I used this.

imagemagick is something you'd typically expect to find in a system package manager

How would you typically know if some dependency could be found in a package manager instead of conda ?

nvm. I found this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That latextools package seems to be more about building and manipulating LaTeX source in Python, rather than actually providing the LaTeX distribution - it still needs TeXLive or something.

Conda often has more than I expect it to, but generally command-line tools unrelated to Python would be installed from the system package manager (because they're system packages). Conda does blur the lines a little bit (especially when it comes to BLAS), but imagemagick and pdflatex would normally be system commands.

Copy link
Member Author

Choose a reason for hiding this comment

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

TeXLive comes with tlmgr (its own package manager for LaTeX files), so a minimal installation for your purposes here would probably be

$ apt-get install imagemagick pdfcrop pdf2svg
$ apt-get install texlive  # fairly minimal version, but I think it has amsmath and amsfonts
$ tlmgr install braket qcircuit

(granted I'm guessing a little, but that's the gist)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks ! I will test this in a new virtual environment when I update the documentation.

I used texlive as well but I had to go on a search for a couple of more latex packages based on the errors.

@jakelishman
Copy link
Member Author

This shouldn't logically affect coverage by a significant amount, because I don't believe this part of the production is tested anyway. Getting a proper installation of TeXLive running on GitHub Actions CI will take quite a long time - TeXLive Full 2021 is something like 5GB, or I suppose you could install TeXLive core, and then tlmgr install the various bits of amsmath, braket and qcircuit as necessary?

@BoxiLi: did you consider setting a threshold on Coveralls' reports of failures? There's an option somewhere around to have it only report failures if they're greater than a given percent.

Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

Tested with the Jupyter notebook of https://nbviewer.ipython.org/github/qutip/qutip-notebooks/blob/master/examples/quantum-gates.ipynb.

LGTM. I shall merge if @purva-thakre find the same on Ubuntu

@BoxiLi
Copy link
Member

BoxiLi commented Jun 18, 2021

did you consider setting a threshold on Coveralls' reports of failures?

Yes, I remember you mentioned this once somewhere during the unitary hack. Haven't implement it yet. That is a good one to include. 0.1% decrease is no failure. It is just because the total number of lines is decreasing. Is that option somewhere already implemented in qutip?

Testing with whole tex library is too heavy indeed.

Edit: Ok I have set the failure threshold to 0.1%

@jakelishman
Copy link
Member Author

Since this file isn't covered, I assume it's actually that the total number of lines went up rather than down. These tools are usually smart enough to call a multiline string one "logical line", but in removing it, I added an extra try/catch block which adds 3 or 4 more lines in total.

@purva-thakre
Copy link
Contributor

LGTM. I shall merge if @purva-thakre find the same on Ubuntu

@BoxiLi Once #63 is merged, the linked notebook is able run all the code blocks properly.

@jakelishman
Copy link
Member Author

By the way, given that SVGs are just text-based, there's probably an approximate way to test some of this functionality - I could imagine a test that constructs a circuit injecting some dummy text into a node somewhere, then calls circuit._repr_svg(), and asserts that dummy_text in svg and "svg" in svg. That's harder to do for the PNG file, but plausible. Of course, that relies on getting a decent LaTeX distribution installed on a CI machine.

@BoxiLi BoxiLi merged commit 7bad281 into qutip:master Jun 23, 2021
BoxiLi pushed a commit to BoxiLi/qutip-qip that referenced this pull request Jul 11, 2021
This file included GPL'd code, which is incompatible with QuTiP's
3-clause BSD licence.  Instead, we rely on the user to have a functional
install of the file (it's been included in TeXLive since at least 2014).

This also attempts to maintain patches made to the LaTeX code.

See qutip/qutip#1579
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