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 tags to tutorials #2467

Merged
merged 14 commits into from
Aug 30, 2024
Merged

Add tags to tutorials #2467

merged 14 commits into from
Aug 30, 2024

Conversation

purva-thakre
Copy link
Contributor

@purva-thakre purva-thakre commented Aug 10, 2024

Description

Fixes #2463

The following are WIP:

  • Need to add Basic, Intermediate and Advanced tags.
  • Couple of tutorial examples show up as jupyter notebooks. If a tag does not exist, the new tag file does not created if it is first used in the jupyter notebook. Discussed it is fine to not have a pyquil tag during the community call
  • Fix a color scheme for frontend, backend, qem technique badges

License

  • I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

@@ -0,0 +1,10 @@
(sphx_tag_bqskit)=
Copy link
Contributor Author

@purva-thakre purva-thakre Aug 13, 2024

Choose a reason for hiding this comment

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

Most of these added files in _tags are causing a few build warnings. Might need to fix some build errors when dependabot upgrades to 9.0.

RemovedInSphinx90Warning: Sphinx 9 will drop support for representing paths as strings. Use "pathlib.Path" or "os.fspath" instead

https://github.com/unitaryfund/mitiq/actions/runs/10363857000/job/28688193465?pr=2467#step:8:212

Copy link
Contributor Author

@purva-thakre purva-thakre Aug 24, 2024

Choose a reason for hiding this comment

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

This warning appears to be an issue that could be fixed by downgrading Sphinx < 8.0

executablebooks/MyST-NB#619

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 14, 2024

We have 2 pyquil tutorials in ipnyb format. If I follow the instructions provided by sphinx-tags, new tags in ipnyb files are not created.

For example, if I have an ipnyb file with tags pyquil and zne, the tag for pyquil is not created in _tags/ as I had not used this tag label before. There were no issues with the zne tag because this file already exists in _tags/. 844df51

https://github.com/unitaryfund/mitiq/actions/runs/10391784801/job/28775505338#step:8:1366

@bdg221
Copy link
Collaborator

bdg221 commented Aug 23, 2024

I know you are working on the functionality, but currently tags are shown on the examples page in the following places:

image

I think having the tags above the examples could be a useful place for it:
image

Copy link

codecov bot commented Aug 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.71%. Comparing base (4046cf6) to head (d6ac335).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
+ Coverage   98.70%   98.71%   +0.01%     
==========================================
  Files          88       89       +1     
  Lines        4091     4131      +40     
==========================================
+ Hits         4038     4078      +40     
  Misses         53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@purva-thakre purva-thakre marked this pull request as ready for review August 24, 2024 16:32
@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 24, 2024

@bdg @cosenal This PR is ready for a review.

Not sure why the RTD build is failing. Will look into this later. You should still be able to verify the changes in a local build.

Force push worked!


Screenshot of previous RTD failure.
image

@bdg221
Copy link
Collaborator

bdg221 commented Aug 26, 2024

@purva-thakre I think the tag implementation is great. My only concern is that a user going to the examples page, will have to take extra steps to use the tags. Is there a way that the links to the tags could be incorporated into the examples page so that they can be accessed without an extra click?

This may just be me nitpicking and if @cosenal and you think the details you provided at the top of the examples page is sufficient, then that's cool with me.

@jordandsullivan
Copy link
Contributor

@bdg221 I agree it would be nice to have the tags at the top of the page, but I think we could do that in a separate issue, unless @purva-thakre thinks it would be easier to just add it to this one.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 26, 2024

The tags become a long list on top of the examples. Which is why I chose to move them to a separate page.

There's no way to list the tags horizontally because they are autogenerated as shown below:

```{toctree}
---
caption: Tags in the documentation tutorials:
maxdepth: 1
---
advanced (11) <advanced>
basic (12) <basic>
bqskit (1) <bqskit>
braket (3) <braket>
calibration (1) <calibration>
cdr (2) <cdr>
cirq (17) <cirq>
ddd (3) <ddd>
intermediate (8) <intermediate>
ionq (1) <ionq>
pec (2) <pec>
pennylane (3) <pennylane>
qibo (1) <qibo>
qiskit (11) <qiskit>
qrack (1) <qrack>
rem (1) <rem>
shadows (2) <shadows>
stim (1) <stim>
zne (24) <zne>
```

@bdg221
Copy link
Collaborator

bdg221 commented Aug 26, 2024

Sounds like there isn't an easy way to get the tags to horizontally appear above the examples and it makes more sense to put that in a separate issue.

Copy link
Collaborator

@bdg221 bdg221 left a comment

Choose a reason for hiding this comment

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

Initial implementation of tags looks good.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 26, 2024

Sounds like there isn't an easy way to get the tags to horizontally appear above the examples and it makes more sense to put that in a separate issue.

@bdg221 Feel free to create the issue and assign it to yourself, if you are interested.

This would involve requesting a new feature in https://github.com/melissawm/sphinx-tags because the examples in the official documentation are also using a vertical list. I chose to use sphinx-tags due to its ability to auto-generate the tags page.

Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

Unless there is a specific reason for it (e.g. you manually adjusted some tag definition), I don't see much value in including the _tags folder in the repo. The only benefit I can think of would be build consistency across different environments. However, we don't version control the docs/build folder anyway, so we may as well do the same for _tags.

@cosenal
Copy link
Contributor

cosenal commented Aug 29, 2024

@bdg221 Did you mean to click on «Request changes»? It's not clear from your comment.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 29, 2024

Unless there is a specific reason for it (e.g. you manually adjusted some tag definition)

@cosenal I had to manually add tagsindex.md to docs/source/examples/tags.md. The latter is then linked on docs/source/examples/examples.md.

@cosenal
Copy link
Contributor

cosenal commented Aug 29, 2024

Unless there is a specific reason for it (e.g. you manually adjusted some tag definition)

@cosenal I had to manually add tagsindex.md to docs/source/examples/tags.md. The latter is then linked on docs/source/examples/examples.md.

You can still do that while gitignoring the _tags folder, no?

@bdg221
Copy link
Collaborator

bdg221 commented Aug 29, 2024

@bdg221 Did you mean to click on «Request changes»? It's not clear from your comment.

No, I didn't mean to. I've cleared it now.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 29, 2024

You can still do that while gitignoring the _tags folder, no?

@cosenal You're correct.

For some reason, I thought the build would fail if I don't have the autogenerated .md files in the _tags folder.

Although every time we run make html, the tool is going to add the .md files.

@natestemen
Copy link
Member

Not sure if this is configurable, but is it possible to make the URL .../tags/... instead of .../_tags/...? It's a vanity thing, but _tags makes me think it's private! 😆

@purva-thakre purva-thakre marked this pull request as draft August 29, 2024 18:14
@purva-thakre
Copy link
Contributor Author

Converting to a draft to look into Nate's comment as well as the issue with make docs adding back the .md files in _tags.

@cosenal
Copy link
Contributor

cosenal commented Aug 30, 2024

I actually like the convention of using underscore in front of "build" folders in order to make them recognizable from folders that contain source files 🤷🏻‍♂️

@purva-thakre
Copy link
Contributor Author

@cosenal Looking through the configuration settings for sphinx-tags, they do not have a setting for not keeping the autogenerated files.

So, even if I deleted the autogenerated files in this PR, someone else running make docs locally will have to make sure to delete these files. We could request this as a feature in the project repo. I know some sphinx tools do have a setting to not keep these files.

@cosenal
Copy link
Contributor

cosenal commented Aug 30, 2024

@purva-thakre the mechanism you just described is .gitignore, which I see you have already modified accordingly :)

Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

LGTM 🏷️

docs/source/examples/tags.md Outdated Show resolved Hide resolved
@natestemen
Copy link
Member

I actually like the convention of using underscore in front of "build" folders in order to make them recognizable from folders that contain source files 🤷🏻‍♂️

Yeah I generally agree with this as a method to indicate on your local machine what's happening. When that pattern shows its face publicly, however, I find strange. My comment was referring to the URL of the tags on the built documentation. E.g. in the current RTD build it reads

mitiq--2467.org.readthedocs.build/en/2467/_tags/qiskit.html

but it would look more natural IMO to be

mitiq--2467.org.readthedocs.build/en/2467/tags/qiskit.html

This is not a hill I'd like to die on, however. Again, just a vanity thing.

While it's a topic of discussion, however, would tag be more appropriate here than tag?

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 30, 2024

@purva-thakre the mechanism you just described is .gitignore, which I see you have already modified accordingly :)

Ah, yes. Initially, I had an incorrect path to _tags in gitignore. This is why the .md files were being added locally whenever I ran make docs. Fixed in 8f8183e

@purva-thakre purva-thakre marked this pull request as ready for review August 30, 2024 13:09
@purva-thakre
Copy link
Contributor Author

purva-thakre commented Aug 30, 2024

This is not a hill I'd like to die on, however. Again, just a vanity thing.

No need to die on this hill @natestemen. Fixed in cc6704a

While it's a topic of discussion, however, would tag be more appropriate here than tag?

Is there a typo in this line?

image

@cosenal
Copy link
Contributor

cosenal commented Aug 30, 2024

Honestly I don't know what I would prefer, whether dying on a hill, or becoming a cow stuck between two tags 😭

.gitignore Outdated Show resolved Hide resolved
@purva-thakre purva-thakre merged commit bb44aa4 into main Aug 30, 2024
18 checks passed
@purva-thakre
Copy link
Contributor Author

Merging this after the community call discussion.

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.

Organize tutorials
5 participants