Skip to content

feat: Add Iceberg architecture builder and annotation - #517

Merged
maximilianruesch merged 15 commits into
mainfrom
mr/feat/iceberg-builder
Nov 4, 2026
Merged

maximilianruesch merged 15 commits into
mainfrom
mr/feat/iceberg-builder

Conversation

@maximilianruesch

Copy link
Copy Markdown
Collaborator

No description provided.

@maximilianruesch
maximilianruesch requested a review from a team as a code owner November 2, 2026 14:58
"""

@dataclass
class OperationCosts:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add a test that the names provided here match the ops provided by the HUGR extension.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean that for every op here there is a hugr op name that matches it?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it should be the opposite: for every hugr op, there is a cost that matches it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Scratch that, the dynamic ops should not have costs, it should be just the block primitives.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I mean we can add such a test (we will have to exclude idle), if you want to stick to the idea.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd like to make sure that the hugr ops and costs don't become disjoint. For example, we're missing pauli_exp_all_x_except_z_at and pauli_exp_all_z_except_x_at here. Annoyingly, the naming is not that consistent:

Cost field Current HUGR extension name
cx_intrablock cx
cx_interblock cx_between_blocks
{xx,yy,zz}_phase_intrablock {xx,yy,zz}_phase
{xx,yy,zz}_phase_interblock {xx,yy,zz}_phase_between_blocks

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fair endeavour. My next question would be what naming to adapt though

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Imo the names we have for the costs fields are more descriptive and having the "default" cx be the intrablock op has no justification.

Comment on lines +40 to +41
HALF_DEPTH = auto()
LOG_DEPTH = auto()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add docstrings to each of these. Depth is related to the block size k.

Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
Comment thread src/guppyft/code/iceberg/encode.py Resolved
@maximilianruesch
maximilianruesch merged commit aa732b7 into main Nov 4, 2026
21 checks passed
@maximilianruesch
maximilianruesch deleted the mr/feat/iceberg-builder branch November 4, 2026 10:19
@hugrbot hugrbot mentioned this pull request Nov 4, 2026
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