qiboteam / qibo

A full-stack framework for quantum computing.
https://qibo.science
Apache License 2.0
294 stars 60 forks source link

Matplotlib circuit drawer #1370

Closed sergiomtzlosa closed 2 months ago

sergiomtzlosa commented 4 months ago

A new class named MPLDrawer to draw the circuit using matplotlib, this is a code integration and enhancement from https://github.com/rpmuller/PlotQCircuit.

The circuit now has a function called draw_mpl() to invoke matplotlib on drawing.

Checklist:

renatomello commented 4 months ago

I think this is a great idea, but models is not the best submodule to insert this functionality.

sergiomtzlosa commented 4 months ago

I think this is a great idea, but models is not the best submodule to insert this functionality.

Thanks a lot for your feedback @renatomello I will move the MPLDrawer to a new single module called imaging as I think this will grow up in the future...

renatomello commented 4 months ago

I think this is a great idea, but models is not the best submodule to insert this functionality.

Thanks a lot for your feedback @renatomello I will move the MPLDrawer to a new single module called imaging as I think this will grow up in the future...

Maybe we should hear some other opinions first, e.g. @scarrazza @MatteoRobbiati @alecandido ?

alecandido commented 4 months ago

@sergiomtzlosa you have about 30 commits with the same message (more exactly 28, of which 26 in two consecutive days).

I'm not against small commits at all, but:

If you can not describe the change, try to group your commits differently, e.g. completing a unit before saving (where a "unit" could be even something temporary, or a step in a bug hunt, not necessarily code ready for production), or reconsider if that code is needed at all (if you don't know how to describe its role, there are chances that it's not that useful after all...).

sergiomtzlosa commented 4 months ago

Thanks a lot for this valuable feedback. I will have a look at them...

sergiomtzlosa commented 4 months ago

Thanks a lot for this work @sergiomtzlosa. It is for sure something which can improve the usability of Qibo in some contexts. Before doing a proper review to this, I just want to open a small discussion the best way to set up this in Qibo. Some time ago I had a discussion with @alecandido where we explored the possibility to add some representation of the circuit through HTML or Matplotlib indeed. At that time we discussed it as a method of the circuit class.

It is also true we typically avoid to add plotting functions in the already existing Qibo files, thus the approach you propose could make sense.

Anyway, thanks a lot for this work :)

Thanks everybody for all comments! Regarding your question, IMHO, to draw the circuit using a function from inside the circuit is a fast way to do it as it is something you would like to see on the screen, it is very convenient, but I understand that you may want to detach the UI functions from the qibo core. I do not have a final opinion on this because both approaches are right.

sergiomtzlosa commented 2 months ago

Hi,

Thanks a lot for your support and advices, I have uploaded the documentation and gave and example on the doc/source path, please, let me know if this is enough of not.

Thanks!