Skip to content

Fix direction handling and incorrect EulerianDigraph requirement in FacialWalks - #931

Open
MeikeWeiss wants to merge 8 commits into
digraphs:mainfrom
MeikeWeiss:meike/planarembedding
Open

MeikeWeiss wants to merge 8 commits into
digraphs:mainfrom
MeikeWeiss:meike/planarembedding

Conversation

@MeikeWeiss

Copy link
Copy Markdown
Contributor

I fixed the issue #930 by really ignoring the direction of the edges. Additionally I fixed the same for FacialWalks and clarified the documentation of DualPlanarGraph. While doing this I realized that FacialWalks requires an EulerianDigraph as an input which is not correct.

@codecov

codecov Bot commented May 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.43%. Comparing base (0fe44fd) to head (f7d5b00).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
gap/attr.gi 50.00% 1 Missing ⚠️
gap/planar.gi 75.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
- Coverage   97.45%   97.43%   -0.02%     
==========================================
  Files          50       50              
  Lines       21188    21193       +5     
  Branches      639      639              
==========================================
+ Hits        20649    20650       +1     
- Misses        474      478       +4     
  Partials       65       65              

☔ View full report in Codecov by Harness.
📢 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.

@james-d-mitchell james-d-mitchell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm happy with this PR, sorry if it has been sitting here waiting for a review for ages! Only one question: is it really necessary to take the symmetric closure of the graph? In many functions in Digraphs we simply assume the graph is symmetric, and return the answer as if it was. Would this be appropriate here too?

@MeikeWeiss

Copy link
Copy Markdown
Contributor Author

Thanks for the input! I changed the handling with symmetric so that it is hopefully more like in other methods.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants