Skip to content

Move all subroutines in star_coloring and acyclic_coloring#239

Closed
amontoison wants to merge 1 commit intomainfrom
merge_subroutines
Closed

Move all subroutines in star_coloring and acyclic_coloring#239
amontoison wants to merge 1 commit intomainfrom
merge_subroutines

Conversation

@amontoison
Copy link
Copy Markdown
Collaborator

For the paper, I explained star_coloring and acyclic_coloring as one algorithm without the subroutines.
Based on our modifications and the additional arguments passed to the subroutines, I am wondering if it might be more relevant to keep just two functions.

@amontoison amontoison requested a review from gdalle April 5, 2025 20:34
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (10239ec) to head (60be001).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #239   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1785      1775   -10     
=========================================
- Hits          1785      1775   -10     

☔ 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.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Apr 5, 2025

I disagree, there are two purposes to the subroutines:

  • avoiding code duplication (treat! is called several times for instance)
  • increasing clarity by separating the conceptual steps of the algorithm

Our code does not have to exactly mirror the paper. When I suggested presenting just one algorithm (the main loop), I meant we could keep the subroutines as subroutines, without recursing into them. I didn't mean they should be unrolled into a giant pseudocode, and thus I don't think we should unroll in our code either. Not to mention it mirrors the structure of the original paper.

@amontoison
Copy link
Copy Markdown
Collaborator Author

It's fine for me. It was just a suggestion.
I'm closing the PR.
I just changed one comment but I will open another PR for that.

@amontoison amontoison closed this Apr 5, 2025
@amontoison amontoison deleted the merge_subroutines branch April 5, 2025 21:06
@amontoison amontoison mentioned this pull request Apr 5, 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.

2 participants