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

more changes to optgap plotting #132

Conversation

PratikSathe
Copy link
Contributor

@PratikSathe PratikSathe commented Jul 11, 2022

  • Added function to metrics.py for computing the empirical probability distribution of cut sizes.
  • Added function that plots the cactus plots
  • Added an mplstyle file
  • Rather than using mplstyle globally, using it with a context manager
  • Restructuring in plot_metrics_optgaps to allow choosing which metrics to plot.

- Added an mplstyle file
- Restructuring in plot_metrics_optgaps to allow choosing which metrics to plot.
- Added function to metrics.py for computing the empirical probability distribution of cut sizes.
- Added function that plots the cactus plots
- mplstyle file updated. Repaired loading the mplstyle file
- Rather than using mplstyle globally, using it with a context manager
@rtvuser1
Copy link
Collaborator

There may be too many changes mixed into this one PR. I'm concerned about the extent of the changes to the plot_volumetric_background function. This is used in many places that you have not yet worked on. I would prefer that the changes for maxcut are isolated form the rest of the plots that are used elsewhere, just to avoid potential side effects. Can you restructure this to limit what is done here ?

Also, the addition of the mplstyle file could have issues (another file that all benchmarks access, and unless it is fully tested across all BM variations, there could be some things that break). It may be fine, but is it necessary to do for this set of changes? I'd prefer to keep the changes isolated and distinct from one another. Modifying how styles are done is independent of the addition of cactus and violin plots, I believe.

Not saying we should not use the style approach, it just complicates testing to do too many things at once, and there are many other parts of the benchmark project that might be affected. E.g. the algorithmic qubits work, or the multiple variations of some of the other plots. Better to keep those untouched while we experiment with the maxcut.

I prefer to hold off merging this with the potential side effects it may cause.

@PratikSathe
Copy link
Contributor Author

PratikSathe commented Jul 12, 2022

I certainly see your concerns. The diff gives the impression that many changes were made, which is actually misleading. Most of my changes to plot_metrics_background are just adding an indentation to a big block to code. This indentation follows with plt.style.context(style_file):, which applies the style file settings only to the indented block of code.

I have not made changes to the plot_volumetric_background function. Perhaps you were referring to plot_metrics_background? I have made no changes to plot_volumetric_background. On the other hand, plot_metrics_background is called by only one function, namely plot_area_metrics. So these changes are indeed isolated from non-MaxCut benchmarks.

I appreciate your concern about the addition of the mplstyle file potentially causing problems with other benchmarks. I had thought about that as well, and hence implemented a solution with effects that are local by design. Specifically, I use temporary styling (matplotlib temporary styling documentation). These settings are not loaded anywhere else; consequently, all other functions behave just as they did previously. The only places which see the style file are the optimality gap plotting, cactus plotting, and area metrics plotting.

My testing shows that no other side effects arise. All other features, such as algorithmic qubits work can proceed independently without issues.

However, I totally understand how the Git diff gives a very long list of changes, which makes it hard to review and understand them. Perhaps we can go through them when we meet? In any case, I can always go back to not using the style file.

@rtvuser1
Copy link
Collaborator

Merging now

@rtvuser1 rtvuser1 merged commit d773694 into SRI-International:add-maxcut-benchmark Jul 12, 2022
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.

None yet

2 participants