-
Notifications
You must be signed in to change notification settings - Fork 61
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
Enabling plot circuit resolution through dpi
argument
#1435
Conversation
@MatteoRobbiati I will remove this from the current milestone, ok? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1435 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 81 81
Lines 11935 11935
=======================================
Hits 11926 11926
Misses 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/qibo/ui/mpldrawer.py
Outdated
@@ -629,18 +632,16 @@ def _plot_params(style: Union[dict, str, None]) -> dict: | |||
return style | |||
|
|||
|
|||
def plot_circuit(circuit, scale=0.6, cluster_gates=True, style=None): | |||
def plot_circuit(circuit, scale=0.6, cluster_gates=True, style=None, dpi=100): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new separate argument, why not directly using the style
one?
In principle, it accepts a dict
working as an update of the PLOT_PARAMS
constant above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, you could also add a **kwargs
and merge it with the style
content, in order to support also the current syntax, with no special handling for any parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is now, the PLOT_PARAMS
dictionary is collecting parameters useful in very different functions/calls of the plotter, putting together arguments of very different and sometimes also unrelated objects.
In this case, for example, the dpi
argument can be passed to plt.figure
, to the plt.savefig
function or set directly into the plt.rcParams
.
We could add it as a new argument of the PLOT_PARAMS
and this is already better than adding an extra argument to the plot_circuit
function indeed.
However, I feel a bit confusing to manage all these (different) tasks using the same dictionary. In particular because for now it is conceived as a tool to customize the style (looking at the example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you already added dpi
to PLOT_PARAMS
before my comment:
Line 28 in 08cf82f
"dpi": 100, |
In any case, there is a single task involved: plt.rcParams
. Ttbomu, PLOT_PARAMS
is just a subset of that, with some defaults set (and then, you have various sets of overrides to pick a certain style). Everything else is just a consequence.
This allows to set the plot resolution.
Checklist: