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.

Sign up for GitHub

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

Jump to bottom

Improve error message in DAGCircuit.draw for invalid filenames #7447

Merged
merged 6 commits into from Jan 14, 2022
Merged

Improve error message in DAGCircuit.draw for invalid filenames #7447

merged 6 commits into from Jan 14, 2022

Conversation

iuliazidaru
Copy link
Contributor

Summary

Fix #3099 dag_drawer should check the existence of filename extension

Details and comments

Added new exception and test

@coveralls
Copy link

coveralls commented Jan 11, 2022 •
edited

Pull Request Test Coverage Report for Build 1698104057

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 83.134%
Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/uc.py 2 87.94%
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 1698101903: 0.03%
Covered Lines: 52017
Relevant Lines: 62570
💛 - Coveralls

jakelishman
jakelishman reviewed Jan 11, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

In general this looks good to me. I had some questions in line, but most stuff is fairly minor.

qiskit/visualization/dag_visualization.py Outdated
Comment on lines 34 to 40
FILENAME_EXTENSIONS = [
"bmp",
"canon",
"cgimage",
"cmap",
"cmapx",
"cmapx_np",
Copy link
Member

Choose a reason for hiding this comment

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

Where did you find this full list? Perhaps we could cut it down to some more common ones - I think things like cmapx_np might be a bit more noise than use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pydot library was throwing a exception with all those extensions...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see - it's the failing output of dot -Tunknown.

qiskit/visualization/dag_visualization.py Outdated Show resolved Hide resolved
qiskit/visualization/dag_visualization.py Show resolved Hide resolved
qiskit/visualization/dag_visualization.py Outdated
if extension not in FILENAME_EXTENSIONS:
raise InvalidFileError(
"Filename extension must be one of: "
+ " ".join([str(elem) for elem in FILENAME_EXTENSIONS])
Copy link
Member

Choose a reason for hiding this comment

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

All the elements in FILENAME_EXTENSIONS are already strings, so this is just the same as " ".join(FILENAME_EXTENSIONS).

test/python/visualization/test_dag_drawer.py Outdated
Comment on lines 42 to 46
try:
dag_drawer(self.dag, filename="aaabc")
self.fail("Expected error not raised!")
except InvalidFileError as exception_instance:
self.assertEqual(
Copy link
Member

Choose a reason for hiding this comment

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

Here, and in the next test, a clearer way of writing these tests of exceptions is

with self.assertRaisesRegex(InvalidFileError, "Parameter 'filename' must be in format 'name.extension'"):
    dag_drawer(self.dag, filename="aaabc")

test/python/visualization/test_dag_drawer.py Outdated
Comment on lines 59 to 63
"Filename extension must be one of: bmp canon cgimage cmap cmapx cmapx_np "
"dot dot_json eps exr fig gd gd2 gif gv icns ico imap imap_np ismap jp2 "
"jpe jpeg jpg json json0 mp pct pdf pic pict plain plain-ext png pov "
"ps ps2 psd sgi svg svgz tga tif tiff tk vdx vml vmlz vrml wbmp webp "
"xdot xdot1.2 xdot1.4 xdot_json",
Copy link
Member

Choose a reason for hiding this comment

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

We might not want to test that exactly this set of extensions are supported - it'll be ok just to test that the message starts with "Filename extension must be one of:"

@jakelishman jakelishman added the Changelog: None Do not include in changelog label Jan 11, 2022
@iuliazidaru
Copy link
Contributor Author

@jakelishman Thank you for your review. I fixed the issues.

javabster
javabster previously approved these changes Jan 13, 2022
View reviewed changes
Copy link
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

This LGTM 🚀 Thanks for your contribution @iuliazidaru!

jakelishman
jakelishman reviewed Jan 13, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Looks good to me! Please could you add a release note explaining the bug fix, and then we'll be good to merge.

jakelishman
jakelishman previously approved these changes Jan 14, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment •
edited

Choose a reason for hiding this comment

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

This all looks good, thanks a lot!

I just reworded the release note a touch to add in Sphinx cross-references to the functions mentioned, and allude to the more common entry point DAGCircuit.draw.

edit: sorry, apparently I was in two minds about the release note - first time round I tagged it as not needing one, then I asked you for one! My bad, sorry!

@jakelishman jakelishman changed the title dag_drawer should check the existence of filename extension Improve error message in DAGCircuit.draw for invalid filenames Jan 14, 2022
@jakelishman jakelishman added automerge Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable and removed Changelog: None Do not include in changelog labels Jan 14, 2022
@jakelishman jakelishman added this to To do in Visualization and Juypter Tools via automation Jan 14, 2022
@jakelishman jakelishman added this to the 0.19.2 milestone Jan 14, 2022
@iuliazidaru
Copy link
Contributor Author

@jakelishman Thanks a lot!

@mergify mergify bot merged commit 25e1d20 into Qiskit:main Jan 14, 2022
Visualization and Juypter Tools automation moved this from To do to Done Jan 14, 2022
mergify bot pushed a commit that referenced this pull request Jan 14, 2022
@iuliazidaru
Improve error message in DAGCircuit.draw for invalid filenames (#7447)
cce3855 
* dag_drawer should check the existence of filename extension

* fix review comments

* add release note

* Reword release note

Co-authored-by: Abby Mitchell <23662430+javabster@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 25e1d20)
@mergify mergify bot mentioned this pull request Jan 14, 2022
Merged
mergify bot added a commit that referenced this pull request Jan 14, 2022
@mergify @iuliazidaru
Improve error message in DAGCircuit.draw for invalid filenames (#7447…
1d5ca45 
…) (#7531)

* dag_drawer should check the existence of filename extension

* fix review comments

* add release note

* Reword release note

Co-authored-by: Abby Mitchell <23662430+javabster@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 25e1d20)

Co-authored-by: Iulia Zidaru <iuliazidaru@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@javabster javabster javabster left review comments

@nkanazawa1989 nkanazawa1989 Awaiting requested review from nkanazawa1989

@nonhermitian nonhermitian Awaiting requested review from nonhermitian nonhermitian is a code owner

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Visualization and Juypter Tools
  
Done
Milestone
0.19.2
Development

Successfully merging this pull request may close these issues.

dag_drawer should check the existence of filename extension
4 participants
@iuliazidaru @coveralls @jakelishman @javabster