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

Add option in user config file to control reverse_bits in circuit_drawer #9150

Closed
diemilio opened this issue Nov 17, 2022 · 11 comments · Fixed by #9211
Closed

Add option in user config file to control reverse_bits in circuit_drawer #9150

diemilio opened this issue Nov 17, 2022 · 11 comments · Fixed by #9211
Assignees
Labels
mod: visualization qiskit.visualization type: feature request New feature or request
Milestone

Comments

@diemilio
Copy link
Contributor

What should we add?

Currently, the user config file supports circuit_drawer display options such as output ("text", "mpl", etc.). Including the ability to change the reverse_bits option would allow users that prefer setting this option to True to not have to explicitly set this parameter every time a circuit is drawn.

@diemilio diemilio added the type: feature request New feature or request label Nov 17, 2022
@diemilio
Copy link
Contributor Author

I would like to work on this issue if you think there is value in adding this option.

@jakelishman
Copy link
Member

If you've got a use-case for it, this definitely seems good to accept! I'll assign the issue to you, thanks, but no pressure.

@jakelishman jakelishman added the mod: visualization qiskit.visualization label Nov 17, 2022
@jakelishman jakelishman added this to the 0.23.0 milestone Nov 17, 2022
@diemilio
Copy link
Contributor Author

diemilio commented Nov 18, 2022

Hi @jakelishman. Thank you so much.

I already have a working version of this, but I have a question before submitting a PR.

While working on this, I realized that very recently, a new option was added to generalize how qubits are ordered when displayed by the drawer (using the wire_order option).

Reading through the issue where this was addressed, I noticed that @1ucian0 proposed to eventually eliminate the reverse_bits option, and just have something like wire_order = 'reverse' take care of it. It was decided not to include this change in the corresponding PR, so I want to check if it is OK for me to move forward without making this change either.

This wouldn't be too difficult to implement, but it is definitely much larger in scope than what I am proposing because removing reverse_bits from the drawer would be a breaking change.

@jakelishman
Copy link
Member

We're fine to move ahead just adding a reverse_bits option to the configuration - even if the exact named option in the function were to go away in the future, it has an easy mapping onto the wire_order form, so the configuration-file option would still make sense.

@diemilio
Copy link
Contributor Author

Hi @jakelishman. I am sorry, this is my first time contributing to qiskit-terra and I am facing a linting issue. Where's a good place to ask about this?

I did some searching, I see this problem is known: #8052 but I don't know what I need to do to resolve it.

Thanks!

@jakelishman
Copy link
Member

For now, you can just delete the lines starting jax and jaxlib from your requirements-dev.txt file (but don't commit those changes), then re-run tox -e lint. Alternatively, you could bypass tox by doing pip install black astroid==2.5.6 pylint==2.8.3 in your development environment, and then running the linters as:

black qiskit test tools examples setup.py
pylint -rn qiskit test tools

In general, I've got an open PR #8967 to split up the actual development requirements from our additional optional things, which will help a fair amount here.

@mtreinish
Copy link
Member

I'm actually surprised there is still an issue with arm64 macOS. Looking at the binaries included in jaxlib there is an arm64 wheel available in the latest release for py3.8-py3.11 https://pypi.org/project/jaxlib/#files

@diemilio
Copy link
Contributor Author

diemilio commented Nov 22, 2022

Hi @jakelishman thank you so much, that worked.

I submitted PR #9186 that takes care of this, but I realized I have some issues with authorship of the first commits. I closed the PR because fixing it probably requires rewriting commit history, so I rather fix this by resubmitting the commits with the right user on a clean branch.

@diemilio
Copy link
Contributor Author

Hi @jakelishman. Thank you for your help today.

Here are a few questions related to the PR:

  1. I noticed that currently there is no checks inside circuit_drawer for invalid values of the reverse_bits option. For example, if reverse_bits is set to something other than a boolean (like the number 5), there is no error raised. reverse_bits just gets set to the default value (False). Is this the expected behavior, or is this something we should fix?

  2. The way I have things now, there is a scenario that might be confusing for the user. Say the user had set the option circuit_reverse_bits = True in the settings.conf file, but happens to forget about it. If they then try to draw a circuit with the wire_order option (let's say, qc.draw(wire_order=[2,1,0]), they will get the error message: "The wire_order option cannot be set when the reverse_bits option is True.", which will be confusing since they are not explicitly passing the reverse_bits option. The question is, which scenario should be implemented:

    • Leave things the way they are. It is up to the user to figure out that they had the reverse_bits option set in the settings.conf file.
    • Display a different error message that explicitly mentions reverse_bits had been set in the config file.
    • Override the circuit_reverse_bits = True setting in config file if wire_order is set, but reverse_bits isn't explicitly passed.
  3. This is a bit tangential, but I noticed there state_drawer option is missing from the set_config function in the user_config file. Also, the test_user_config is missing tests for the state_drawer. Not sure if these should be done in a separate PR, but just wanted to bring it up.

Thanks again!

@jakelishman
Copy link
Member

Some answers:

  1. I wouldn't worry about this - it's pretty much part of Python's philosophy. If anything the value of 5 should make reverse_bits be True (since 5 is truth-y), but I really wouldn't worry much.
  2. This is a really nice observation, thanks! It's quite usual in these sorts of cases to have the "more local" options take precedence. In this case, that means a wire_order specified in the function call should override a reverse_bits specified in the environment, or in a configuration file. If it's not too messy, I'd have the logic do that (and not issue any errors or warnings to the user).
  3. Probably best for a separate PR - thanks for bringing it up!

@diemilio
Copy link
Contributor Author

diemilio commented Dec 10, 2022

Thanks @jakelishman.

  1. Makes sense, thanks.
  2. This would be very easy to implement. I just need to change the order of two lines of code. I'm looking into this. I still don't think it is too complicated to implement, but it is not as simple as I originally thought. Done.
  3. Sounds good. I am happy to help with this, just want to make sure it is big enough of a change to submit a PR.

@mergify mergify bot closed this as completed in #9211 Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment