HugrView: validate nodes, and remove Base #523

acl-cqc · 2023-09-12T12:21:55Z

Move valid_node and valid_non_root from HugrMutInternals into HugrView
HugrView provides default implementations of get_parent, get_nodetype, get_optype and get_metadata that perform node-validity checks; remove the other implementations.
Add DEFAULT_NODETYPE + DEFAULT_OPTYPE constants
Change HierarchyView::new->try_new, add explicit checks that root node is valid in the base view.
Remove manual implementations of Clone for SiblingGraph + DescendantsGraph, just derive
Remove the Base parameter to SiblingGraph/DescendantsGraph - instead parameterize only constructor (via : impl) and cache the base's .base_hugr() rather than the base.

…th valid_node checks ==> SiblingGraph::new's "get_optype" will implicitly check the root is in the base view. This does mean some unnecessary index checking for &Hugr's. We could continue to reimplement those methods for AsRef<Hugr> if we want to avoid this inefficiency. Alternatively we could make UnmanagedDenseMap return Options rather than unwrapping. Some other ugly alternative/hacks for returning default values - DEFAULT_{NODE,OP}TYPE

…e_hugr in c'tor

lmondada

Very nice clean up! I like it a lot.

Personally the only thing that still bothers me a bit in this code is the HierarchyView trait. I have always found it a weird trait, with as its only method a new constructor. I wonder if we can get rid of it, or maybe give it a more meaningful name. Maybe ViewFromRef or something...

Any opinions? Unless we can find a quick compromise here, this is probably the stuff of another PR at some point in the future. Thanks again for the work!

lmondada · 2023-09-12T15:48:32Z

src/hugr/views.rs

+    fn valid_node(&self, node: Node) -> Result<(), HugrError> {
+        match self.contains_node(node) {
+            true => Ok(()),
+            false => Err(HugrError::InvalidNode(node)),
+        }
+    }
+
+    /// Validates that a node is a valid root descendant in the graph.
+    ///
+    /// To include the root node use [`HugrMutInternals::valid_node`] instead.
+    ///
+    /// Returns a [`HugrError::InvalidNode`] otherwise.
+    #[inline]
+    fn valid_non_root(&self, node: Node) -> Result<(), HugrError> {


I'd prefer having a verb phrase as function name (eg is_valid_node or validate_node), but feel free to ignore this subjective comment. I don't think we have a unified style opinion on this.

But maybe this would be confused with a fn that returns bool, so not sure. valid_node_or_err might be yet another option.

Possibly for another PR but I agree valid_node is...grammatically very dubious ;-). As you say, is_valid_node would return a bool, but validate_node is a possibility. OTOH we have that method name in validation already and it means something very different (!). How about check_node_in_graph, check_has_node?

You are right, using validate is maybe confusing. Your suggestions are good. My last attempt: check_contains_node and check_contains_non_root.

I like check_contains_node....and could just be check_non_root. Think renaming can be a second PR tho?

lmondada · 2023-09-12T15:55:20Z

src/hugr/views.rs

-    fn get_metadata(&self, node: Node) -> &NodeMetadata;
+    #[inline]
+    fn get_metadata(&self, node: Node) -> &NodeMetadata {
+        // The other way to do it - exploit the UnmanagedDenseMap's get() returning &default


This comment is a bit cryptic to me, I'm not sure what it means.

Oh, sorry, this is where I'm not sure what to do. Hopefully @aborgna-q can give a shout, I'm wondering about a PR to add something to portgraph's UnmanagedDenseMap....

Alan's referring to the behaviour of the metadata container, which returns a reference to the default value if it doesn't know about the node index.

I'd avoid fiddling with portgraph assumptions when you can directly

static DEFAULT_METADATA: NodeMetadata = NodeMetadata::Null; match self.contains_node(node) { true => self.base_hugr().metadata.get(node.index), false => &DEFAULT_METADATA, }

Ok, I can use NodeMetadata::Null, indeed.

So I guess this will do for now - I don't think this have to declare const DEFAULT_NODETYPE etc. is a good general solution (when I know an appropriately-lifetimed default value is sitting there in the map), but it's just about OK for the specific cases here. IOW I might come back to this in another PR later...

src/hugr/views/sibling.rs

lmondada · 2023-09-12T16:01:32Z

src/hugr/views/sibling.rs

    /// The chosen root node.
    root: Node,

    /// The filtered portgraph encoding the adjacency structure of the HUGR.
    graph: FlatRegionGraph<'g>,

-    /// The rest of the HUGR.
-    hugr: &'g Base,
+    /// View onto the underlying Hugr which this graph filters


Thanks for the improved doc!

Is "view" the correct term though? I would have just said "The underlying Hugr" or "reference to..."

lmondada · 2023-09-12T16:04:20Z

src/hugr/views.rs

+    ///
+    /// # Errors
+    /// Returns [`HugrError::InvalidNode`] if the root isn't a node of the required [OpTag]
+    fn new(hugr: &'a impl HugrView, root: Node) -> Result<Self, HugrError>;


Again, naming questions that I am not the authority on but will still give my opinion -- use it as you wish. I try to prefix function names that can fail with try_, so here I would choose try_new.

@aborgna-q what is your preference? You're more of a rustacean than me.

Yep, this should be try_new

Sounds good to me, done. I wonder about a utility function new that calls try_new(...).unwrap() - even if that were #[cfg(test)]. I have not done the latter ATM but please shout if you find repeated try_new...unwrap sufficiently annoying to justify this idea!

…aseless

## 🤖 New release * `quantinuum-hugr`: 0.1.0 <details><summary>Changelog</summary> <blockquote> ## 0.1.0 (2024-01-15) ### Bug Fixes - Subgraph boundaries with copies ([#440](#440)) - [**breaking**] Use internal tag for SumType enum serialisation ([#462](#462)) - Check kind before unwrap in insert_identity ([#475](#475)) - Allow for variables to get solved in inference ([#478](#478)) - In IdentityInsertion add noop to correct parent ([#477](#477)) - Failing release tests ([#485](#485)) - [**breaking**] Serialise `Value`, `PrimValue`, and `TypeArg` with internal tags ([#496](#496)) - Serialise custom constants with internal tag ([#502](#502)) - [**breaking**] Reduce max int width in arithmetic extension to 64 ([#504](#504)) - HugrView::get_function_type ([#507](#507)) - TODO in resolve_extension_ops: copy across input_extensions ([#510](#510)) - Use given input extensions in `define_function` ([#524](#524)) - Lessen requirements for hugrs in outline_cfg ([#528](#528)) - Make unification logic less strict ([#538](#538)) - Simple replace incorrectly copying metadata ([#545](#545)) - Account for self-referencial constraints ([#551](#551)) - Consider shunted metas when comparing equality ([#555](#555)) - Join labels in issue workflow ([#563](#563)) - Comment out broken priority code ([#562](#562)) - Handling of issues with no priority label ([#573](#573)) - Don't insert temporary wires when extracting a subgraph ([#582](#582)) - Improve convexity checking and fix test ([#585](#585)) - Ignore input->output links in SiblingSubgraph::try_new_dataflow_subgraph ([#589](#589)) - Enforce covariance of SiblingMut::RootHandle ([#594](#594)) - Erratic stack overflow in infer.rs (live_var) ([#638](#638)) - Work harder in variable instantiation ([#591](#591)) - Actually add the error type to prelude ([#672](#672)) - Serialise dynamically computed opaqueOp signatures ([#690](#690)) - FuncDefns don't require that their extensions match their children ([#688](#688)) - Binary compute_signature returning a PolyFuncType with binders ([#710](#710)) - Use correct number of args for int ops ([#723](#723)) - [**breaking**] Add serde tag to TypeParam enum ([#722](#722)) - Allow widening and narrowing to same width. ([#735](#735)) - Case node should not have an external signature ([#749](#749)) - [**breaking**] Normalize input/output value/static/other ports in `OpType` ([#783](#783)) - No dataflow_signature for block types ([#792](#792)) - Ignore unsupported test in miri ([#794](#794)) - Include schema rather than read file ([#807](#807)) ### Documentation - Add operation constraint to quantum extension ([#543](#543)) - Coverage check section in DEVELOPMENT.md ([#648](#648)) - Remove "quantum extension" from HUGR spec. ([#694](#694)) - Improve crate-level docs, including example code. ([#698](#698)) - Spec cleanups and clarifications ([#742](#742)) - Spec clarifications ([#738](#738)) - Spec updates ([#741](#741)) - [spec] Remove references to causal cone and Order edges from Input ([#762](#762)) - Mention experimental inference in readme ([#800](#800)) - Collection of spec updates for 0.1 ([#801](#801)) - Add schema v0 ([#805](#805)) - Update spec wrt. polymorphism ([#791](#791)) ### Features - Derive things for builder structs ([#229](#229)) - Return a slice of types from the signature ([#238](#238)) - Move `dot_string` to `HugrView` ([#271](#271)) - [**breaking**] Change `TypeParam::USize` to `TypeParam::BoundedNat` and use in int extensions ([#445](#445)) - TKET2 compatibility requirements ([#450](#450)) - Split methods between `HugrMut` and `HugrMutInternals` ([#441](#441)) - Add `HugrView::node_connections` to get all links between nodes ([#460](#460)) - Location information in extension inference error ([#464](#464)) - Add T, Tdg, X gates ([#466](#466)) - [**breaking**] Add `ApplyResult` associated type to `Rewrite` ([#472](#472)) - Implement rewrite `IdentityInsertion` ([#474](#474)) - Instantiate inferred extensions ([#461](#461)) - Default DFG builders to open extension sets ([#473](#473)) - Some helper methods ([#482](#482)) - Extension inference for conditional nodes ([#465](#465)) - Add ConvexChecker ([#487](#487)) - Add clippy lint for mut calls in a debug_assert ([#488](#488)) - Default more builder methods to open extension sets ([#492](#492)) - Port is serializable ([#489](#489)) - More general portgraph references ([#494](#494)) - Add extension deltas to CFG ops ([#503](#503)) - Implement petgraph traits on Hugr ([#498](#498)) - Make extension delta a parameter of CFG builders ([#514](#514)) - [**breaking**] SiblingSubgraph does not borrow Hugr ([#515](#515)) - Validate TypeArgs to ExtensionOp ([#509](#509)) - Derive Debug & Clone for `ExtensionRegistry`. ([#530](#530)) - Const nodes are built with some extension requirements ([#527](#527)) - Some python errors and bindings ([#533](#533)) - Insert_hugr/insert_view return node map ([#535](#535)) - Add `SiblingSubgraph::try_from_nodes_with_checker` ([#547](#547)) - PortIndex trait for undirected port parameters ([#553](#553)) - Insert/extract subgraphs from a HugrView ([#552](#552)) - Add `new_array` operation to prelude ([#544](#544)) - Add a `DataflowParentID` node handle ([#559](#559)) - Make TypeEnum and it's contents public ([#558](#558)) - Optional direction check when querying a port index ([#566](#566)) - Extension inference for CFGs ([#529](#529)) - Better subgraph verification errors ([#587](#587)) - Compute affected nodes for `SimpleReplacement` ([#600](#600)) - Move `SimpleReplace::invalidation_set` to the `Rewrite` trait ([#602](#602)) - [**breaking**] Resolve extension ops (mutating Hugr) in (infer_and_->)update_validate ([#603](#603)) - Add accessors for node index and const values ([#605](#605)) - [**breaking**] Expose the value of ConstUsize ([#621](#621)) - Nicer debug and display for core types ([#628](#628)) - [**breaking**] Static checking of Port direction ([#614](#614)) - Builder and HugrMut add_op_xxx default to open extensions ([#622](#622)) - [**breaking**] Add panicking integer division ops ([#625](#625)) - Add hashable `Angle` type to Quantum extension ([#608](#608)) - [**breaking**] Remove "rotations" extension. ([#645](#645)) - Port.as_directed to match on either direction ([#647](#647)) - Impl GraphRef for PetgraphWrapper ([#651](#651)) - Provide+implement Replace API ([#613](#613)) - Require the node's metadata to always be a Map ([#661](#661)) - [**breaking**] Polymorphic function types (inc OpDefs) using dyn trait ([#630](#630)) - Make prelude error type public ([#669](#669)) - Shorthand for retrieving custom constants from `Const`, `Value` ([#679](#679)) - [**breaking**] HugrView API improvements ([#680](#680)) - Make FuncDecl/FuncDefn polymorphic ([#692](#692)) - [**breaking**] Simplify `SignatureFunc` and add custom arg validation. ([#706](#706)) - [**breaking**] Drop the `pyo3` feature ([#717](#717)) - [**breaking**] `OpEnum` trait for common opdef functionality ([#721](#721)) - MakeRegisteredOp trait for easier registration ([#726](#726)) - Getter for `PolyFuncType::body` ([#727](#727)) - `Into<OpType>` for custom ops ([#731](#731)) - Always require a signature in `OpaqueOp` ([#732](#732)) - Values (and hence Consts) know their extensions ([#733](#733)) - [**breaking**] Use ConvexChecker trait ([#740](#740)) - Custom const for ERROR_TYPE ([#756](#756)) - Implement RemoveConst and RemoveConstIgnore ([#757](#757)) - Constant folding implemented for core and float extension ([#769](#769)) - Constant folding for arithmetic conversion operations ([#720](#720)) - DataflowParent trait for getting inner signatures ([#782](#782)) - Constant folding for logic extension ([#793](#793)) - Constant folding for list operations ([#795](#795)) - Add panic op to prelude ([#802](#802)) - Const::from_bool function ([#803](#803)) ### HugrMut - Validate nodes for set_metadata/get_metadata_mut, too ([#531](#531)) ### HugrView - Validate nodes, and remove Base ([#523](#523)) ### Miscellaneous Tasks - [**breaking**] Update portgraph 0.10 and pyo3 0.20 ([#612](#612)) - [**breaking**] Hike MSRV to 1.75 ([#761](#761)) ### Performance - Use lazy static definittion for prelude registry ([#481](#481)) ### Refactor - Move `rewrite` inside `hugr`, `Rewrite` -> `Replace` implementing new 'Rewrite' trait ([#119](#119)) - Use an excluded upper bound instead of max log width. ([#451](#451)) - Add extension info to `Conditional` and `Case` ([#463](#463)) - `ExtensionSolution` only consists of input extensions ([#480](#480)) - Remove builder from more DFG tests ([#490](#490)) - Separate hierarchy views ([#500](#500)) - [**breaking**] Use named struct for float constants ([#505](#505)) - Allow NodeType::new to take any Into<Option<ExtensionSet>> ([#511](#511)) - Move apply_rewrite from Hugr to HugrMut ([#519](#519)) - Use SiblingSubgraph in SimpleReplacement ([#517](#517)) - CFG takes a FunctionType ([#532](#532)) - Remove check_custom_impl by inlining into check_custom ([#604](#604)) - Insert_subgraph just return HashMap, make InsertionResult new_root compulsory ([#609](#609)) - [**breaking**] Rename predicate to TupleSum/UnitSum ([#557](#557)) - Simplify infer.rs/report_mismatch using early return ([#615](#615)) - Move the core types to their own module ([#627](#627)) - Change &NodeType->&OpType conversion into op() accessor ([#623](#623)) - Infer.rs 'fn results' ([#631](#631)) - Be safe ([#637](#637)) - NodeType constructors, adding new_auto ([#635](#635)) - Constraint::Plus stores an ExtensionSet, which is a BTreeSet ([#636](#636)) - [**breaking**] Remove `SignatureDescription` ([#644](#644)) - [**breaking**] Remove add_op_<posn> by generalizing add_node_<posn> with "impl Into" ([#642](#642)) - Rename accidentally-changed Extension::add_node_xxx back to add_op ([#659](#659)) - [**breaking**] Remove quantum extension ([#670](#670)) - Use type schemes in extension definitions wherever possible ([#678](#678)) - [**breaking**] Flatten `Prim(Type/Value)` in to parent enum ([#685](#685)) - [**breaking**] Rename `new_linear()` to `new_endo()`. ([#697](#697)) - Replace NodeType::signature() with io_extensions() ([#700](#700)) - Validate ExtensionRegistry when built, not as we build it ([#701](#701)) - [**breaking**] One way to add_op to extension ([#704](#704)) - Remove Signature struct ([#714](#714)) - Use `MakeOpDef` for int_ops ([#724](#724)) - [**breaking**] Use enum op traits for floats + conversions ([#755](#755)) - Avoid dynamic dispatch for non-folding operations ([#770](#770)) - Simplify removeconstignore verify ([#768](#768)) - [**breaking**] Unwrap BasicBlock enum ([#781](#781)) - Make clear const folding only for leaf ops ([#785](#785)) - [**breaking**] `s/RemoveConstIgnore/RemoveLoadConstant` ([#789](#789)) - Put extension inference behind a feature gate ([#786](#786)) ### SerSimpleType - Use Vec not TypeRow ([#381](#381)) ### SimpleReplace+OutlineCfg - Use HugrMut methods rather than .hierarchy/.graph ([#280](#280)) ### Testing - Update inference test to not use DFG builder ([#550](#550)) - Strengthen "failing_sccs_test", rename to "sccs" as it's not failing! ([#660](#660)) - [**breaking**] Improve coverage in signature and validate ([#643](#643)) - Use insta snapshots to add dot_string coverage ([#682](#682)) - Miri ignore file-opening test ([#684](#684)) - Unify the serialisation tests ([#730](#730)) - Add schema validation to roundtrips ([#806](#806)) ### `ConstValue - :F64` and `OpaqueOp::new` ([#206](#206)) ### Cleanup - Remove outdated comment ([#536](#536)) ### Cosmetic - Format + remove stray TODO ([#444](#444)) ### Doc - Crate name as README title + add reexport ([#199](#199)) ### S/EdgeKind - :Const/EdgeKind::Static/ ([#201](#201)) ### Simple_replace.rs - Use HugrMut::remove_node, includes clearing op_types ([#242](#242)) ### Spec - Remove "Draft 3" from title of spec document. ([#590](#590)) - Rephrase confusing paragraph about TailLoop inputs/outputs ([#567](#567)) ### Src/ops/validate.rs - Common-up some type row calculations ([#254](#254)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustín Borgna <[email protected]>

acl-cqc requested review from aborgna-q and lmondada September 12, 2023 12:21

acl-cqc force-pushed the new/hugr_view_valid_baseless branch from d74ccf7 to a4dee2c Compare September 12, 2023 14:41

acl-cqc added 6 commits September 12, 2023 15:59

Move valid_node/valid_non_root into HugrView (no uses yet)

e28c60a

Remove Base type-parameter to SiblingGraph/DescendantsGraph, call bas…

17c3a30

…e_hugr in c'tor

Derive Clone for SiblingGraph + DescendantsGraph

8475d12

HierarchyView::new returns Result/HugrError

aa88f75

test

487c795

acl-cqc force-pushed the new/hugr_view_valid_baseless branch from a4dee2c to 487c795 Compare September 12, 2023 15:51

acl-cqc changed the title ~~[Slightly WIP] HugrView: validate nodes, and remove Base~~ HugrView: validate nodes, and remove Base Sep 12, 2023

lmondada reviewed Sep 12, 2023

View reviewed changes

acl-cqc mentioned this pull request Sep 12, 2023

Add SiblingMut allowing modification of a SiblingGraph #522

Merged

acl-cqc added 5 commits September 12, 2023 18:08

Merge remote-tracking branch 'origin/main' into new/hugr_view_valid_b…

ea70450

…aseless

Remove debug println

430e4e6

use NodeMetadata::Null

c789969

correct comment re. view/filter

6507bbc

new -> try_new

929ecf4

lmondada approved these changes Sep 13, 2023

View reviewed changes

acl-cqc added this pull request to the merge queue Sep 13, 2023

Merged via the queue into main with commit 35a0ef0 Sep 13, 2023
6 checks passed

acl-cqc deleted the new/hugr_view_valid_baseless branch September 13, 2023 16:23

github-actions bot mentioned this pull request Jan 3, 2024

chore: release v0.1.0 #766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HugrView: validate nodes, and remove Base #523

HugrView: validate nodes, and remove Base #523

acl-cqc commented Sep 12, 2023 •

edited

Loading

lmondada left a comment •

edited

Loading

lmondada Sep 12, 2023

acl-cqc Sep 12, 2023

lmondada Sep 13, 2023

acl-cqc Sep 13, 2023

lmondada Sep 12, 2023

acl-cqc Sep 12, 2023

aborgna-q Sep 13, 2023

acl-cqc Sep 13, 2023

lmondada Sep 12, 2023

lmondada Sep 12, 2023

aborgna-q Sep 13, 2023

acl-cqc Sep 13, 2023 •

edited

Loading

HugrView: validate nodes, and remove Base #523

HugrView: validate nodes, and remove Base #523

Conversation

acl-cqc commented Sep 12, 2023 • edited Loading

lmondada left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

acl-cqc commented Sep 12, 2023 •

edited

Loading

lmondada left a comment •

edited

Loading

acl-cqc Sep 13, 2023 •

edited

Loading