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

Rename NotADag #137

Closed
cqc-alec opened this issue Jun 8, 2023 · 6 comments · Fixed by #157
Closed

Rename NotADag #137

cqc-alec opened this issue Jun 8, 2023 · 6 comments · Fixed by #157
Assignees

Comments

@cqc-alec
Copy link
Collaborator

cqc-alec commented Jun 8, 2023

Validation can fail for reasons other than not being a directed acyclic graph, so the name is misleading. Perhaps InvalidGraph?

@aborgna-q
Copy link
Collaborator

aborgna-q commented Jun 8, 2023

That's a bug in the Dag checker with multiports 😅. I have a fix in the works.

@cqc-alec
Copy link
Collaborator Author

cqc-alec commented Jun 8, 2023

If it's a bug in the DAG checker then it's also a documentation bug:

    #[error("The children of an operation {optype:?} must form a dag with single source and sink. Loops are not allowed, nor are dangling nodes not in the path between the input and output. In node {node:?}.")]
    NotADag { node: Node, optype: OpType },

(The docs already imply that there are more conditions than not being a DAG.)

@aborgna-q
Copy link
Collaborator

The "single source and sink" requirement is an special kind of DAG. I think we are still in the NotADag category, and the user should be able to see the docs or the error message with the specific info.

@cqc-alec
Copy link
Collaborator Author

cqc-alec commented Jun 8, 2023

Just seems odd to get an error called NotADag for something that is in fact a DAG, just not the right kind of DAG.

@aborgna-q
Copy link
Collaborator

Fair enough.

I'd go with something more specific rather than more generic tough, InvalidGraph doesn't communicate much about the failing check.
NotASingleSinkSourceDag seems like a mouthful :P

@cqc-alec
Copy link
Collaborator Author

cqc-alec commented Jun 8, 2023

Hmm...

https://cs.stackexchange.com/questions/67849/what-do-you-call-a-dag-with-a-single-root-source

How about NotABoundedDAG? "Bounded" because if we interpret it as a poset then it has a maximal and a minimal element?

@aborgna-q aborgna-q self-assigned this Jun 12, 2023
ss2165 added a commit that referenced this issue Nov 21, 2024
Closes #136

drive-by: cargo update (hugr 0.13.2)

I have tested the test in
quantinuum-dev/guppy-integration#19 now passes and the
example has expected behaviour with crz oracle.
ss2165 pushed a commit that referenced this issue Nov 21, 2024
## 🤖 New release
* `hugr-llvm`: 0.6.0 -> 0.6.1 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.6.1](CQCL/hugr-llvm@v0.6.0...v0.6.1) -
2024-10-23

### Bug Fixes

- don't normalise half turns
([#137](CQCL/hugr-llvm#137))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
ss2165 added a commit that referenced this issue Nov 22, 2024
Closes #136

drive-by: cargo update (hugr 0.13.2)

I have tested the test in
quantinuum-dev/guppy-integration#19 now passes and the
example has expected behaviour with crz oracle.
ss2165 pushed a commit that referenced this issue Nov 22, 2024
* `hugr-llvm`: 0.6.0 -> 0.6.1 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

2024-10-23

- don't normalise half turns
([#137](CQCL/hugr-llvm#137))
</blockquote>

</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
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 a pull request may close this issue.

2 participants