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

docs: Provide docs for array ops, fix bad doc for HugrView::poly_func_type #2021

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

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Mar 24, 2025

No description provided.

Copy link

codecov bot commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.09%. Comparing base (30c038d) to head (cf2f366).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2021      +/-   ##
==========================================
- Coverage   83.79%   83.09%   -0.71%     
==========================================
  Files         211      215       +4     
  Lines       39763    40895    +1132     
  Branches    36435    37109     +674     
==========================================
+ Hits        33320    33981     +661     
- Misses       4567     5030     +463     
- Partials     1876     1884       +8     
Flag Coverage Δ
python 85.84% <ø> (-6.23%) ⬇️
rust 82.81% <ø> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

swap,
/// separates leftmost element from the rest of the array.
/// `pop_left<SIZE><elemty>: array<SIZE, elemty> -> Option<elemty, array<SIZE-1,elemty>>`
Copy link
Contributor Author

@acl-cqc acl-cqc Mar 24, 2025

Choose a reason for hiding this comment

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

Note that if SIZE is 0, the u64 will (presumably) wrap, so the result type will be an Option<array<U64_MAX, elemty>> which will be None at runtime.

Given SIZE must be statically known, we could remove the Option:

  • pop_left<0> could return Option of anything we want, or raise error in compute_signature
  • pop_left<1> could return just the element (no Option nor array)
  • pop_left<x> when x>=2 could return the element and rest of array (no Option)

and similarly for pop_right

Copy link
Member

Choose a reason for hiding this comment

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

this seems like a return type you can't write down without a conditional, which is note very nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, yes, OK maybe I've gone too far. See also #1521 though: even if we returned the element and a size-0 array in all cases N>=1, we could still avoid the option by erroring in compute_signature for N==0. A lesser step but a good one?

Which is to say, I'll update/correct #1521 a bit and let's go from there

@acl-cqc acl-cqc changed the title doc: Provide docs for array ops, fix bad doc for HugrMut::poly_func_type doc: Provide docs for array ops, fix bad doc for HugrView::poly_func_type Mar 24, 2025
@acl-cqc acl-cqc changed the title doc: Provide docs for array ops, fix bad doc for HugrView::poly_func_type docs: Provide docs for array ops, fix bad doc for HugrView::poly_func_type Mar 24, 2025
@acl-cqc acl-cqc marked this pull request as ready for review March 24, 2025 11:10
@acl-cqc acl-cqc requested a review from a team as a code owner March 24, 2025 11:10
@acl-cqc acl-cqc requested review from jake-arkinstall and ss2165 and removed request for jake-arkinstall March 24, 2025 11:10
set,
/// exchanges two indices within the array: `swap<size,elemty>: array<size, elemty>, index index -> either(array, array)`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// exchanges two indices within the array: `swap<size,elemty>: array<size, elemty>, index index -> either(array, array)`
/// exchanges two indices within the array: `swap<size,elemty>: array<size, elemty>, index, index -> either(array, array)`

swap,
/// separates leftmost element from the rest of the array.
/// `pop_left<SIZE><elemty>: array<SIZE, elemty> -> Option<elemty, array<SIZE-1,elemty>>`
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a return type you can't write down without a conditional, which is note very nice

swap,
/// separates leftmost element from the rest of the array.
Copy link
Member

Choose a reason for hiding this comment

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

mention None indicates failure?

@acl-cqc acl-cqc requested a review from ss2165 March 25, 2025 15:38
Copy link
Collaborator

@doug-q doug-q left a comment

Choose a reason for hiding this comment

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

Nice. I think these strings belong in fn description(), but this is surely an improvement so approving.

new_array,
/// copies an element out of the array ([TypeBound::Copyable] elements only):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// copies an element out of the array ([TypeBound::Copyable] elements only):
/// Copies an element out of the array ([TypeBound::Copyable] elements only):

get,
/// exchanges an element of the array with an external value:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// exchanges an element of the array with an external value:
/// Exchanges an element of the array with an external value:

set,
/// exchanges two indices within the array:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// exchanges two indices within the array:
/// Exchanges two indices within the array:

swap,
/// separates leftmost element from the rest of the array:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// separates leftmost element from the rest of the array:
/// Separates leftmost element from the rest of the array:

pop_left,
/// separates rightmost element from the rest of the array.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// separates rightmost element from the rest of the array.
/// Separates rightmost element from the rest of the array.

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.

5 participants