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

Notebook for adding input layers and operators #275

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lkct
Copy link
Member

@lkct lkct commented Oct 7, 2024

As per title.

Also some minor changes:

@lkct lkct added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 7, 2024
@lkct lkct requested a review from loreloc October 7, 2024 11:27
@lkct lkct self-assigned this Oct 7, 2024
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@1d4f8f3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
cirkit/symbolic/layers.py 80.00% 1 Missing and 1 partial ⚠️
cirkit/symbolic/parameters.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #275   +/-   ##
=======================================
  Coverage        ?   71.24%           
=======================================
  Files           ?       45           
  Lines           ?     4782           
  Branches        ?      708           
=======================================
  Hits            ?     3407           
  Misses          ?     1176           
  Partials        ?      199           

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

@loreloc
Copy link
Member

loreloc commented Oct 7, 2024

Thanks a lot!

I think the how-to-add-an-operator notebook has lower priority and does not need to be merged soon, so I just have some suggestions for the how-add-a-layer notebook:

  • Do we need to write how to add an operator over layers too? I think this deserves a separate notebook. At this stage, it is sufficient to simply show how to add a new layer, i.e., symbolic + PyTorch implementation.
  • For registering the compilation rules, I think it is better to show the usage of the PipelineContext rather than updating the global rule dictionary. See add_layer_compilation_rule method https://github.com/april-tools/cirkit/blob/poly_diff_notebook/cirkit/pipeline.py#L64-L65 .

@lkct
Copy link
Member Author

lkct commented Oct 8, 2024

Do we need to write how to add an operator over layers too?

I would say yes, because it's also important to make full use of cirkit. But they indeed face different levels of users.

I think this deserves a separate notebook.

I agree. There're some duplicates in the two notebooks, and we may do it in three: add-layer, layer-op, and circuit-op.

See add_layer_compilation_rule method

Thanks for pointing out. This will be better for end-users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants