Skip to content

Conversation

@aborgna-q
Copy link
Collaborator

Closes #2158

Annoyingly, we still need to return a FlatRegion (or some other concrete wrapper) here so that references to the graph implements some of petgraph's traits (like IntoNodeIdentifiers) which are normally implemented for &'_ G. There's no way to write that in a GAT constraint :/
We'll have to wait for the big petgraph rewrite to clean that.

BREAKING CHANGE: HugrInternals::region_portgraph now also returns a node mapping between hugr and portgraph nodes

@aborgna-q aborgna-q requested a review from lmondada May 7, 2025 13:13
@aborgna-q aborgna-q requested a review from a team as a code owner May 7, 2025 13:13
@aborgna-q aborgna-q requested review from mark-koch and removed request for a team May 7, 2025 13:13
@hugrbot
Copy link
Collaborator

hugrbot commented May 7, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_associated_type_added.ron

Failed in:
trait associated type hugr_core::hugr::internal::HugrInternals::RegionPortgraph in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/internal.rs:28
trait associated type hugr_core::hugr::internal::HugrInternals::RegionPortgraphNodes in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/internal.rs:37

@codecov
Copy link

codecov bot commented May 7, 2025

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.10%. Comparing base (8e36cdc) to head (3370bf9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hugr-core/src/hugr/views/impls.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2164   +/-   ##
=======================================
  Coverage   82.10%   82.10%           
=======================================
  Files         227      227           
  Lines       39795    39805   +10     
  Branches    35894    35904   +10     
=======================================
+ Hits        32672    32682   +10     
  Misses       5301     5301           
  Partials     1822     1822           
Flag Coverage Δ
python 85.64% <ø> (ø)
rust 81.72% <95.45%> (+<0.01%) ⬆️

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.

Copy link
Contributor

@lmondada lmondada left a comment

Choose a reason for hiding this comment

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

Looks amazing, thanks @aborgna-q!

I've tried to remove the methods

    #[inline]
    fn to_portgraph_node(&self, node: impl NodeHandle<Self::Node>) -> portgraph::NodeIndex {
        node.node().into_portgraph()
    }
    #[inline]
    fn from_portgraph_node(&self, index: portgraph::NodeIndex) -> Self::Node {
        index.into()
    }

in HugrInternals and all their uses, but I've hit a roadblock in the implementation of HugrMut::insert_from_view. This turns the view into a portgraph view and then inserts that.

Do you have a plan on how to rewrite that? It's not obvious to me how an implementation of insert_from_view would look if we remove the fn portgraph method. In any case, it seems big enough to be its own PR.

@aborgna-q
Copy link
Collaborator Author

There's a TODO in insert_hugr_internal:

TODO: We may need to replicate portgraph::LinkMut::insert_graph at the
hugr level to avoid calling hierarchy() here.

// TODO: We may need to replicate `portgraph::LinkMut::insert_graph` at the
// hugr level to avoid calling `hierarchy()` here.
let other_portgraph =

We'll need to implement that to remove those uses :/

Base automatically changed from ab/entrypoints to main May 8, 2025 12:30
@aborgna-q aborgna-q force-pushed the ab/region-pg-nodemap branch from b070f39 to 3370bf9 Compare May 8, 2025 12:34
@aborgna-q aborgna-q added this pull request to the merge queue May 8, 2025
Merged via the queue into main with commit 531fe69 May 8, 2025
27 checks passed
@aborgna-q aborgna-q deleted the ab/region-pg-nodemap branch May 8, 2025 12:50
This was referenced May 29, 2025
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.

HugrInternals::region_portgraph should return a map from/to portgraph node idxs

5 participants