search

microsoft / QuantumKatas

Tutorials and programming exercises for learning Q# and quantum computing
MIT License
4.54k stars 1.22k forks source link

Add a task to BasicGates #827

Closed remilvus closed 2 years ago

remilvus commented 2 years ago

This PR addresses #818

Manvi-Agrawal commented 2 years ago

Hmm, I know this is still a work in progress but I am quite intrigued by cell IDs generated by jupyter on your system. This was fixed by me in https://github.com/microsoft/QuantumKatas/pull/825.

I can see that your branch does not contain the fix. Could you please do the following?

Also, could you please share nbformat version which you are using locally? Thanks

Manvi-Agrawal commented 2 years ago

Overall, your changes look good. I left some suggestions for you which you can apply by clicking on "Apply change" button to let Github do the changes automatically for you. Some of these changes are minor suggestion like fixing typos or some rewording. Thanks for working on this.

remilvus commented 2 years ago

Hmm, I know this is still a work in progress but I am quite intrigued by cell IDs generated by jupyter on your system. This was fixed by me in #825.

I can see that your branch does not contain the fix. Could you please do the following?

  • Merge branch main to your branch.
  • Reopen Basic Gates Workbook and save it and see if IDs go away.

Also, could you please share nbformat version which you are using locally? Thanks

I'm using 5.3.0 version of nbformat locally, but now I see you downgraded it to the 4.x version. Thanks for the steps to fix this problem.

Edit: I ended up using a regex to remove the IDs

Manvi-Agrawal commented 2 years ago

Hi @remilvus, I was also on nbformat 5.3. On my system downgrading nbformat version fixed this problem. Then I opened the notebook in jupyter and saved it again to fix this problem.

Can you please open the workbook in jupyter locally, save it and see if it's still generating ids? If it does, can you please try running "Restart kernel and clear all outputs" and see if it fixes the problem.

remilvus commented 2 years ago

Hi @remilvus, I was also on nbformat 5.3. On my system downgrading nbformat version fixed this problem. Then I opened the notebook in jupyter and saved it again to fix this problem.

Can you please open the workbook in jupyter locally, save it and see if it's still generating ids? If it does, can you please try running "Restart kernel and clear all outputs" and see if it fixes the problem.

I didn't try clearing outputs but restarting the kernel didn't remove the IDs. Merging the main branch to downgrade nbformat helped by preventing the IDs from being generated after I manually removed them.

Manvi-Agrawal commented 2 years ago

I didn't try clearing outputs but restarting the kernel didn't remove the IDs. Merging the main branch to downgrade nbformat helped by preventing the IDs from being generated after I manually removed them.

@remilvus, I am glad to know that my fix works. So, hopefully new contributors and the existing ones should not face this annoying issue of automatic cell ids if they have the changes in PR https://github.com/microsoft/QuantumKatas/pull/825, since we dont check in cell ids in this repo.

@tcNickolas, we got some validation here that it works. :-)

remilvus commented 2 years ago

Thanks for all the suggestions @Manvi-Agrawal! Now the PR should be ready.

I have some doubts about hiding the ControlledOnInt function under a hint, as teaching about this function is the main purpose of this task. On the other hand, a task explaining ControlledOnInt and asking the learner to use it would be a little too trivial to solve. In the current format, this task teaches mainly the concept behind a ControlledOnInt(0, <gate>) use case, which I think is more important than deeply explaining ControlledOnInt. The main reason is, that explaining exactly how the ControlledOnInt function works would almost be a repetition of the explanation for ControlledOnBitString. It would be good to hear different opinions though 😃

Manvi-Agrawal commented 2 years ago

@remilvus, I will let @tcNickolas answer the question about introducing the function as a hint.

I guess we have an item in our roadmap to have some Katas focused on introducing Q# to the learners. There was an interest by "Tony Holdroyd" on this one in https://github.com/microsoft/QuantumKatas/issues/542#issuecomment-92416987.

Would you be willing to collaborate with him? For some initial guidance, you can read comments by "tcNickolas(Mariia)" after that question. @tcNickolas thoughts?

remilvus commented 2 years ago

@Manvi-Agrawal I'll be happy to help. I may not have enough time after the academic year start though.

Could you help me with figuring out why the checks are not passing? There are validation errors for the GraphColoring notebook, which I didn't modify. Have you seen similar errors before?

Manvi-Agrawal commented 2 years ago

@remilvus, yes I have seen such errors before. Sometimes we might see failures in other Katas which we don't modify. This is because some of the tests in this repository have a passing probability of approx 99%( Eg: some tests in Graph coloring kata) So, once in a while they might fail. You can trigger CI by pushing an empty commit and it should pass second time

remilvus commented 2 years ago

@Manvi-Agrawal thanks for the explanation. Wouldn't it be useful to mention that this can happen in the CONTRIBUTING document? For example, under "Validating your changes" -> "3. Continuous integration"

tcNickolas commented 2 years ago

For the transient CI build failures, once we finish https://github.com/microsoft/QuantumKatas/pull/620, we should be able to address the root cause of many of the failures by switching the time-consuming tests to the new SparseSimulator. They are the tests that deal with Grover's search and quantum oracles, and this type of programs benefits from sparse simulation greatly, so timeouts should become much less frequent.

remilvus commented 2 years ago

Thanks for making the last fixes!