Skip to content

Conversation

@VictorPrins
Copy link
Contributor

No description provided.

@Wojtek242 Wojtek242 requested review from AckslD and Wojtek242 October 2, 2019 07:30
@Wojtek242 Wojtek242 self-assigned this Oct 2, 2019
Copy link

@Wojtek242 Wojtek242 left a comment

Choose a reason for hiding this comment

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

Very good Victor!

I only made a few small comments. Basically:

  • Axel needs to approve all your deprecation changes
  • I like your use of enums, they are much better at self documenting

Also, you will need to make the CI build pass - I assume that means you need to make all the old tests pass with your new changes. If you have time, please also add tests for your new changes. If you don't let me know about that.

@AckslD
Copy link
Member

AckslD commented Oct 9, 2019

Hi @VictorPrins! Could you rebase to the newest develop to fix the merge conflicts?

Copy link
Member

@AckslD AckslD left a comment

Choose a reason for hiding this comment

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

@VictorPrins nice work! I made some comments, mostly smaller ones. Let me know if you have time to look over these. FYI, I made some comments which start with "Nit:", those are just smaller things which you can just ignore if you don't agree.

One thing I'm missing are tests and one or two examples for how to use the new functionality. I think you already have some examples in your thesis. Could you add that to here? Have you also written some tests? I cannot find these.

cqc/pythonLib.py Outdated

if self._cqc.pend_messages:

# If we are inside a TP_MIX, then insert the CQC Type header before the command header
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to put these things in functions since they can then be reused in _two_qubit_gate, measure reset etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean only these two lines?

if self._cqc._inside_cqc_mix:
    self._cqc._pend_type_header(CQCType.COMMAND, CQCCmdHeader.HDR_LENGTH + CQCAssignHeader.HDR_LENGTH)

Copy link
Member

Choose a reason for hiding this comment

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

There is also this:

            # Build command header and rotation sub header
            command_header = CQCCmdHeader()
            command_header.setVals(self._qID, command, notify, block)

and this

            # Pend headers
            self._cqc._pend_header(command_header)
            self._cqc._pend_header(rot_sub_header)

copied code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@VictorPrins
Copy link
Contributor Author

I have processed and responded to all comments. As we agreed in the email exchange, we will write tests once a test framework has been created in the CQC-Python repo. I will write some examples later this week.

@AckslD
Copy link
Member

AckslD commented Oct 23, 2019

@VictorPrins If I try to run the example https://github.com/SoftwareQuTech/CQC-Python/tree/master/examples/pythonLib/wstate I get a the error

cqc.pythonLib.CQCUnsuppError: Sequence not supported

@VictorPrins
Copy link
Contributor Author

The source of the error is that arbitrary rotations are not implemented in the quantum engine: see this line.

This error is always raised if the rotation gates are invoked. The "Sequence not supported" line is misleading; sequences don't have anything to do with this error. The error is not due to my changes, because the same error is raised if you execute the example in the master branch.

@AckslD
Copy link
Member

AckslD commented Oct 24, 2019

@VictorPrins Ah right! Sorry my bad :)

@AckslD AckslD merged commit 2c207fe into SoftwareQuTech:Develop Oct 25, 2019
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.

3 participants