hugr icon indicating copy to clipboard operation
hugr copied to clipboard

feat: Add DynamicTopoConvexChecker and tests

Open gluonhiggs opened this issue 6 months ago • 2 comments

Resolve issue #2240

Description

I have added the logic for DynamicTopoConvexCheck in the convexity.rs based on the paper (instead of leveraging petgraph's OrderMap because it may require extra modifications to petgraph and is less flexible). I have also adapted it to the repository, and a variety of simple test cases is covered.

gluonhiggs avatar May 30 '25 01:05 gluonhiggs

Codecov Report

Attention: Patch coverage is 86.20690% with 88 lines in your changes missing coverage. Please review.

Project coverage is 82.21%. Comparing base (ede8e7b) to head (e5321e4). Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
hugr-core/src/convexity.rs 86.20% 84 Missing and 4 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2276      +/-   ##
==========================================
+ Coverage   82.15%   82.21%   +0.06%     
==========================================
  Files         239      240       +1     
  Lines       42989    43627     +638     
  Branches    38901    39539     +638     
==========================================
+ Hits        35319    35870     +551     
- Misses       5679     5762      +83     
- Partials     1991     1995       +4     
Flag Coverage Δ
python 85.34% <ø> (ø)
rust 81.89% <86.20%> (+0.07%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar May 30 '25 08:05 codecov[bot]

Hi @gluonhiggs, thanks for your contribution!

I should be able to get to your PR later today :) You'll hear from me!

lmondada avatar May 30 '25 09:05 lmondada

Hi @gluonhiggs, thank you again for your contribution! I'm very glad you've looked at this problem.

There is currently a flaw in your PR: because of an unimplemented function (fn port_to_node), your code logic in fn is_convex is skipped (unless both inputs and outputs are empty) and the method will always return true. See my comments below.

On a more high-level note: can I ask you to open this same PR but in the portgraph repo? As you've designed this ConvexChecker, there is no reason for this to be in Hugr and not in portgraph. I could copy your files over myself, but it's better if you do it, so you get the GitHub attribution you deserve. Apologies for the extra hassle.

Thanks again for your effort and I look forward to your fixes!

@lmondada I have created a PR here on portgraph. I thought it was simply just copy paste from hugr to portgraph :joy:

gluonhiggs avatar Jun 02 '25 17:06 gluonhiggs

Thank you!

lmondada avatar Jun 03 '25 07:06 lmondada