-
Notifications
You must be signed in to change notification settings - Fork 9
feat: PersistentHugr implements HugrView #2202
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2202 +/- ##
==========================================
- Coverage 82.03% 81.99% -0.04%
==========================================
Files 234 237 +3
Lines 41702 42767 +1065
Branches 37616 38679 +1063
==========================================
+ Hits 34209 35068 +859
- Misses 5518 5706 +188
- Partials 1975 1993 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
08e3eb0
to
7cd85d7
Compare
I've rebased on top of main and have simplified/improved the logic to find linked incoming/outgoing ports. In doing so, I've also established a more stable API for querying the boundary edges of Apologies I think this will require a complete re-review. The combination of the changes + rebase makes diff-ing changes between before and now difficult. The main changes are the methods
|
Question: I've implemented a partial subset of The alternative would be to introduce a new trait that captures the parts of Let me know which approach you prefer |
btw this is also the reason why coverage is low. Everything else is well-covered. I could add dummy for the missing implementations to increase coverage artificially, but I don't think that's a useful thing to do... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
I only have some nitpicks
portgraph::view::FlatRegion<'_, Self::RegionPortgraph<'_>>, | ||
Self::RegionPortgraphNodes, | ||
) { | ||
// TODO: this is currently not very efficient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this would require a custom portgraph implementation.
Open an issue and link it from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> #2248 (same below)
// TODO: A lot of these implementations (especially the ones relating to node | ||
// hierarchies) are very inefficient as they (often unnecessarily) construct | ||
// the whole extracted HUGR in memory. We are currently prioritizing correctness | ||
// and clarity over performance and will optimise some of these operations in | ||
// the future as bottlenecks are encountered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Open an issue!
Co-authored-by: Agustín Borgna <[email protected]>
Co-authored-by: Agustín Borgna <[email protected]>
Closes #2095