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

Port single-photon-interference tutorial #79

Merged
merged 5 commits into from
Mar 17, 2024
Merged

Conversation

rochisha0
Copy link
Contributor

Pull Request Description

What does this PR do?

This PR ports a new tutorial on single photon interference for both v4 and v5

Related Issues:

@Ericgig
Copy link
Member

Ericgig commented Jan 23, 2024

Thank you for your interest in contributing to qutip.

Where does this example comes from?
From a quick look, it does not seems use qutip feature that much...

@rochisha0
Copy link
Contributor Author

@Ericgig Thank you for the comment. It is one of the notebooks suggested to port (4th one) by @hodgestar in the issue #18

@hodgestar
Copy link
Contributor

@Ericgig: This notebook was the result of an undergraduate student project that I supervised.

@hodgestar
Copy link
Contributor

@rochisha0 Thank you for opening the PR. It looks good, but there are a few things we need to sort out:

  • The notebooks currently fail the isort and black formatting checks. Could you run isort and black on them and push the changes to this PR? You'll be able to see whether they pass the checks by looking at the results of the builds linked to from the PR page. If you need help using black and isort on notebooks, there are instructions in the README.

  • Currently the tests fail, probably because of some issue with recent Cython or SciPy changes. I'll restart the test runs now and if they fail @Ericgig or I can try sort those out separately.

  • Good idea putting the notebook in its own new folder. We still need to add the new folder to the web page that lists the tutorials. You can find the Jinja template and scripts that generate that in the website folder. We should also probably choose a better folder name than miscellaneous but I haven't thought of a good one yet. I will try come up with a good name (suggestions welcomed).

@rochisha0
Copy link
Contributor Author

@hodgestar Thank you for the feedback. I have made the changes to the formatting style of the notebook. I hope it works. I will make the changes to the website template once we figure out a good folder name.

@rochisha0
Copy link
Contributor Author

@hodgestar How does physics tutorials sound for the folder name?

@hodgestar
Copy link
Contributor

@rochisha0 On second thoughts, let's stick with miscellaneous. At least it will give us somewhere to put this and other once-off tutorials until we see a pattern emerging.

@hodgestar
Copy link
Contributor

It looks like there is still one linting error:

notebooks/miscellaneous/single-photon-interference.ipynb:cell_2:1:1: F821 undefined name 'Image'

@rochisha0
Copy link
Contributor Author

@hodgestar Looks like there were some manual changes to be made regarding comments and images in the notebooks that were not supported by black and isort. I have made the necessary changes to the template, but I cannot find any build information to cross-check that.

@christian512
Copy link
Collaborator

I can not trigger a workflow (guess I'm missing some permission). But I have checked your notebooks using flake8 --max-line-length=88 (as in our linting checks) and it all seems good!

@hodgestar I think this is ready for merging! :)

@hodgestar
Copy link
Contributor

Thanks @christian512. I don't know why the CI didn't run, but let me merge this and see what happens.

@hodgestar hodgestar merged commit ff08a53 into qutip:main Mar 17, 2024
@hodgestar
Copy link
Contributor

@rochisha0 The ported tutorial is now published at http://qutip.org/qutip-tutorials/ -- thank you!

I noticed that there are few minor things that need fixing on the published page that are related to this PR:

  • The new "Miscellaneous" section is missing from the index at the top (the one immediately after "The following are the contents of this page:")
  • The title of the notebook in the "Miscellaneous" section list is "HV Basis". I'm guessing this is because the logic for picking up the title is subtly wrong.

Would you be up to making a new PR to address these?

@rochisha0
Copy link
Contributor Author

@hodgestar Yes, I'll take this up.

@hodgestar
Copy link
Contributor

Thanks, @rochisha0!

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.

4 participants