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

Enable repeat use of UCCDefault1 and update docs #268

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

bachase
Copy link
Collaborator

@bachase bachase commented Feb 27, 2025

Addresses #261 by setting up the PassManager in the UCCDefault initializer rather than on each run. The second commit updates the corresponding examples in the docs to reflect current module structure, converts the examples to Sphix doctest to ensure examples we can automate checking they run properly with new code changes, and adds a doctest check to the Test workflow to do that automation.

@bachase bachase changed the title Draft: Enable repeat use of UCCDefault1 and update docs Enable repeat use of UCCDefault1 and update docs Feb 27, 2025
@bachase
Copy link
Collaborator Author

bachase commented Feb 27, 2025

Checking if the read the docs build automatically does the doctest run, otherwise will add a github workflow step for that.

@bachase bachase force-pushed the 261-ucc-default-and-docs branch from 559cf98 to 46c55f3 Compare February 27, 2025 14:44
@bachase bachase marked this pull request as draft February 27, 2025 15:00
@bachase bachase changed the title Enable repeat use of UCCDefault1 and update docs Draft: Enable repeat use of UCCDefault1 and update docs Feb 27, 2025
@bachase bachase force-pushed the 261-ucc-default-and-docs branch 3 times, most recently from 1ecf5ae to 45c1474 Compare February 27, 2025 16:44
@bachase bachase changed the title Draft: Enable repeat use of UCCDefault1 and update docs Enable repeat use of UCCDefault1 and update docs Feb 27, 2025
@bachase bachase force-pushed the 261-ucc-default-and-docs branch from 45c1474 to 866df24 Compare February 27, 2025 16:49
@bachase bachase marked this pull request as ready for review February 27, 2025 18:29
Copy link
Collaborator

@jordandsullivan jordandsullivan left a comment

Choose a reason for hiding this comment

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

Mostly looks great, just a couple questions.

@@ -40,3 +40,7 @@ jobs:

- name: Run tests
run: poetry run pytest ucc --verbose

- name: Run doctest
# Check that code examples in docs execute as expected, and treat warnings as errors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice addition!

@@ -8,7 +8,3 @@ This page details the publicly accessible functions available in ``ucc``.

.. autoclass:: ucc.transpilers.ucc_defaults.UCCDefault1
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this latest change, I think the API doc looks a little bare. Do we want to leave it to #246 and
Screenshot 2025-02-27 at 11 01 14 AM
to define these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rephrasing to be sure I'm following -- suggesting to remove this section for now as there's not much detail, and then bring it back as part of #246 ?

If so, then yes, I agree that makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that's my suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated with the suggested changes and docs look good, so merging now.

@bachase bachase force-pushed the 261-ucc-default-and-docs branch from 866df24 to fe3f2e2 Compare February 27, 2025 19:17
@bachase bachase merged commit a2d710b into main Feb 27, 2025
2 checks passed
@bachase bachase deleted the 261-ucc-default-and-docs branch February 27, 2025 19:31
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.

2 participants