feat!: Cleanup core trait definitions #2126
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-rs-v0.16.0 #2126 +/- ##
======================================================
- Coverage 83.43% 83.37% -0.07%
======================================================
Files 219 219
Lines 42200 42281 +81
Branches 38302 38383 +81
======================================================
+ Hits 35211 35252 +41
- Misses 5173 5209 +36
- Partials 1816 1820 +4
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:
|
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
|
Uh, the new pub trait HugrInternals {
/// The underlying portgraph view type.
type Portgraph<'p>: LinkView<LinkEndpoint: Eq> + Clone + 'p
where
Self: 'p;is an associated type bound, which was stabilized in The options here are to do CQCL/portgraph#204 and make a breaking release there, or to bump our MSRV... |
| self.mu_out | ||
| .iter() | ||
| .try_for_each(|e| match self.replacement.valid_non_root(e.src) { | ||
| self.mu_out.iter().try_for_each(|e| { | ||
| match check_valid_non_root(&self.replacement, e.src) { |
There was a problem hiding this comment.
Heads up that these changes will probably conflict with #2070, which is about to be merged in. It should be easy to resolve though.
hugr-core/src/hugr/internal.rs
Outdated
|
|
||
| /// Returns the portgraph [Hierarchy](portgraph::Hierarchy) of the graph | ||
| /// returned by [`HugrInternals::portgraph`]. | ||
| #[inline] | ||
| fn hierarchy(&self) -> Cow<'_, portgraph::Hierarchy> { | ||
| Cow::Borrowed(&self.base_hugr().hierarchy) | ||
| /// Returns a flat portgraph view of a region in the HUGR. | ||
| /// | ||
| /// This is a subgraph of [`HugrInternals::portgraph`], with a flat hierarchy. | ||
| fn region_portgraph( | ||
| &self, | ||
| parent: Self::Node, | ||
| ) -> portgraph::view::FlatRegion<'_, Self::Portgraph<'_>> { | ||
| let pg = self.portgraph(); | ||
| let root = self.to_portgraph_node(parent); | ||
| portgraph::view::FlatRegion::new_without_root(pg, self.hierarchy(), root) | ||
| } |
There was a problem hiding this comment.
The following comment is annoying, but unfortunately important for my usecase:
We are deprecating base_hugr---this is excellent for PersistentHugr, as I no longer need to create new Hugrs in memory.
However, if we still expose portgraph() and region_portgraph(), we won't get around the need for a new wrapper struct (call it PGView) to provide an implementation of LinkView for PersistentHugr. This is a problem as LinkEndpoint must implement Into<PortIndex>. In this case, it would require a conversion (HugrID, NodeID) -> u32, which leaves us with 16 bits each for HugrID and NodeID... I'm worried this isn't enough.
What do you think?
There was a problem hiding this comment.
The same comment applies to hierarchy()... The problem with these methods is that they are "global", whereas the only accessor methods that are easy to implement on a PersistentHugr must be local to the neighbourhood of a node.
There was a problem hiding this comment.
After removing some default impls, we now only use hierarchy() for
- Verification
- Some extra cases that'll get dropped with HUGR root pointers #2029
#1 is an issue I'll tackle in a different PR (it's also one of the last callers of base_hugr, so it needs some fixing), and the other one shouldn't be an issue.
Should I maybe just remove the method? (probs do the same as with base_hugr and deprecate it for now)
There was a problem hiding this comment.
As for the node index problem, it is annoying but the only solution I see is to make indices generic in portgraph (as is the case already in petgraph). That should be doable, but I'm not sure if it'll fit in this release :/
hugr-core/src/hugr/validate.rs
Outdated
| let region = self.hugr.region_portgraph(parent); | ||
| let entry_node = self.hugr.children(parent).next().unwrap(); | ||
| dominators::simple_fast(®ion.as_petgraph(), entry_node) | ||
| dominators::simple_fast(®ion, entry_node.pg_index()) |
There was a problem hiding this comment.
suggestion: rename pg_index -> to_portgraph_node for consistency?
f0738b1
Moves some methods around in `HugrInternals` / `HugrView` that we'll require for #1926 and #2029. Notable changes: - Adds a `hierarchy` method to `HugrInternals` that replaces most calls to `base_hugr`. - `HugrInternals::base_hugr` is now deprecated. It's only used by `Sibling/DescendantGraph`, `validate` and a random use in `DeadCodeElimPass`. - Adds a `HugrInternals::region_portgraph` method that returns a `FlatRegion` portgraph wrapper, and a `HugrView::descendants` call. These lets us replace most uses of `SiblingGraph` and `DescendantGraph`. - Renamed `HugrInternals::{get_pg_index,get_node}` to `to_portgraph_node` and `from_portgraph_node`. This requires some new changes in `portgraph`. I'll make a minor release and update it here before merging. We should be able to remove `base_hugr` after #2029. The deprecation warning here is only temporary. BREAKING CHANGE: Modified multiple core `HugrView` and `HugrInternals` trait methods. See #2126.
Moves some methods around in
HugrInternals/HugrViewthat we'll require for #1926 and #2029. Notable changes:hierarchymethod toHugrInternalsthat replaces most calls tobase_hugr.HugrInternals::base_hugris now deprecated. It's only used bySibling/DescendantGraph,validateand a random use inDeadCodeElimPass.HugrInternals::region_portgraphmethod that returns aFlatRegionportgraph wrapper, and aHugrView::descendantscall. These lets us replace most uses ofSiblingGraphandDescendantGraph.HugrInternals::{get_pg_index,get_node}toto_portgraph_nodeandfrom_portgraph_node.This requires some new changes in
portgraph. I'll make a minor release and update it here before merging.We should be able to remove
base_hugrafter #2029. The deprecation warning here is only temporary.BREAKING CHANGE: Modified multiple core
HugrViewandHugrInternalstrait methods. See #2126.