qutip / qutip-qip

The QuTiP quantum information processing package
https://qutip-qip.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
116 stars 63 forks source link

Allow setting dpi/density when exporting circuits to PNG. #133

Closed cgranade closed 2 years ago

cgranade commented 2 years ago

Currently, the density for exporting circuits to PNG is hard-coded to 100dpi (https://github.com/qutip/qutip-qip/blob/master/src/qutip_qip/circuit_latex.py#L122). It'd be really helpful to be able to configure that and export higher-resolution circuit diagrams when appropriate. Thanks! ♥

srinathkp commented 2 years ago

Hi @cgranade / @BoxiLi , I'm interested in qutip project for GSoC'22. As this is marked as a good first issue, I'd like to work on this. I see that we use imagemagick to convert to png and that the density is hard-coded to 100 now. Where can we take the dpi/density argument from ?

BoxiLi commented 2 years ago

Hi @srinathkp, great! Currently the plot is only used for plotting in the notebook https://github.com/qutip/qutip-qip/blob/497fa2f999297e7df6ae5497cc78d6f09bf4cac6/src/qutip_qip/circuit.py#L2055-L2066 Hence there is no argument taken.

You can consider adding an method like QubitCircuit.draw(file_type="png", dpi=100) that outputs a figure file.

srinathkp commented 2 years ago

Hi @BoxiLi / @cgranade , the converter functions for pdf, png and svg are made with fixed configurations(like hard-coded dpi value) using circuit_latex._make_converter(configuration). We can add an element to _CONVERTER_CONFIGURATIONS list with density value from the user but it will mean that a new converter has to be made every time user tries to export an image. A more dynamic approach would be having a new function that directly calls ImageMagick tool with arguments from the user ( with appropriate checks and validation ofc ).

Branch : https://github.com/srinathkp/qutip-qip/tree/feature/export-png-with-dpi

Please let me know your thoughts.

BoxiLi commented 2 years ago

Hi @srinathkp, yes I agree with your argument. Could you make a PR using that branch? It would be easier to review because the PR shows directly what has been changed.

srinathkp commented 2 years ago

Hey @BoxiLi , Thanks for the feedback, Wanted to discuss here before I implement and test it. Will raise a PR when I'm done.

BoxiLi commented 2 years ago

In the current way, the dpi is written fixed in the configuration of the converter https://github.com/qutip/qutip-qip/blob/497fa2f999297e7df6ae5497cc78d6f09bf4cac6/src/qutip_qip/circuit_latex.py#L142-L157

I guess you meant that we can allow an additional argument here for the generated converter. E.g. dpi etc. I think compared to allowing arbitrary arguments, it is better to just allow the dpi e.g. converter(file_name, dpi). Allowing arbitrary argument is more general but it means that the user needs to now how to use ImageMagick. And it makes it harder to raise a proper error message when something goes wrong.

It would be nice if the code does not introduce too much complication as this part has been particularly hard to debug and test. Especially because ImageMagick comments are slightly different among different platforms.

srinathkp commented 2 years ago

Yeah, instead of making a converter with user defined dpi every time, We can do the conversion directly with ImageMagick and save the file when user calls this function

https://github.com/srinathkp/qutip-qip/blob/6ba13079ac17f14ede63c15e4b3b8f08e23b90f0/src/qutip_qip/circuit.py#L2070 def draw(self,file_type="png",dpi=100,file_name="exported_pic",file_path="",alpha=False):

We'll abstract the ImageMagick part from the user so he/she doesn't need to know what it is. Just the dpi value here and the conversion is done and saved. Is this approach good to go or I'm overlooking any potential complications that might show up later @BoxiLi

BoxiLi commented 2 years ago

Yeah, sounds good.

If you encounter anything while you are playing with it, you are always welcome to add more error messages here and there :)

srinathkp commented 2 years ago

Hi @BoxiLi , I created a PR with the changes - #139 . The workflow checks are pending without approval. Can you take a look