qutip / qutip-notebooks

A collection of IPython notebooks using QuTiP: examples, tutorials, development test, etc.
BSD 3-Clause "New" or "Revised" License
238 stars 236 forks source link

Update the notebook using qutip-qip #131

Closed BoxiLi closed 3 years ago

BoxiLi commented 3 years ago
review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

quantshah commented 3 years ago

Hi @BoxiLi, thanks for the updates. I actually ran the notebooks and updated one of them myself. I made a mistake and pushed a new branch on the qutip-notebook repo instead of yours. I will remove it. But good to know that I can directly author commits in the PR. A few points that I took care of in examples/qip-customize-device.ipynb:

I will go through all the notebooks and fix some of the things if you are okay with it.

BoxiLi commented 3 years ago

Sure, feel free to make update. Thanks

The set_up_ops function uses num_qubits which is already defined for the Processor class and we can refer to it as self.num_qubits. However we need to call set_up_ops after we initialise self.num_qubits. This should be corrected in the arXiv example too.

Yes it is good to refer to self.num_qubits before set_up_ops. Actually, if one calls super().__init__ then self.num_qubits is actually already specified there. So in principle, there is no need to define self.num_qubits again. But I think it is good to define it also explicitly here again before set_up_ops, just to make it clear so that people don't necessarily need to dig into the source code to understand it.

BTW, it would be great if we can get this merged this week or by the beginning of next week, because I plan to use it with Mybinder as a demonstration at the end of next week.

quantshah commented 3 years ago

@BoxiLi I have updated the two notebooks and reviewed the other changes. Please pull the changes from this PR locally, run the notebooks and push again. You can simply do

git pull origin qutip_qip where origin points to your own repo.

After that we can merge this. I just had a few minor changes to the code. The major one being adding docstrings and having all the imports at the top of the notebook. Other minor changes are in the writing to make some points clearer.

BoxiLi commented 3 years ago

Great, thanks!

One quick question, why do you prefer having all importation at the beginning rather than importing them right before we use them? I did it in the second way because I thought it would be easier to follow. As we can explain right after what we imported.

BTW, the warning is because of a bug in IPython. You can pin the version ipython==7.10 or ipykernel>=6 to remove it if you like to.

quantshah commented 3 years ago

For the imports, pep8 has a recommendation: https://www.python.org/dev/peps/pep-0008/#imports. I guess I am just too used to seeing all the imports at the beginning. Also, it makes sure that if people want to copy and reuse parts of the code, they can do a bit more easily since they do not have to search the whole notebook to see where something was imported.

Thanks for resolving the Ipython bug warning. Could you push a clean version of the notebooks so that this can be merged? I am making a point release tomorrow and maybe we can also update the notebooks in the website with these new ones.

BoxiLi commented 3 years ago

Also, it makes sure that if people want to copy and reuse parts of the code, they can do a bit more easily since they do not have to search the whole notebook to see where something was imported.

Make sense

Could you push a clean version of the notebooks so that this can be merged?

Sure, I'll get this done.