Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made feedback set algorithm not ignore the nodes post cycle. #5539

Merged
merged 1 commit into from
May 12, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented May 9, 2024

Fixes #5524


This change is Reviewable

@orizi orizi force-pushed the orizi/fix-missing-feedback-set-vertices branch 2 times, most recently from fde84c2 to 588a8b0 Compare May 11, 2024 10:52
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @orizi and @yuvalsw)


crates/cairo-lang-utils/src/graph_algos/feedback_set.rs line 33 at r2 (raw file):

    /// reached, it indicates it's in some cycle that was not "resolved" yet.
    in_flight: UnorderedHashSet<Node::NodeId>,
    /// Node already visited in the current run.

Suggestion:

/// Already visited nodes in the current run.

@orizi orizi force-pushed the orizi/fix-missing-feedback-set-vertices branch from 588a8b0 to 04b7d03 Compare May 12, 2024 03:54
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@orizi orizi force-pushed the orizi/fix-missing-feedback-set-vertices branch from 04b7d03 to 2f97689 Compare May 12, 2024 06:43
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@orizi orizi added this pull request to the merge queue May 12, 2024
Merged via the queue into main with commit 6ae4d3c May 12, 2024
43 checks passed
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions


crates/cairo-lang-utils/src/graph_algos/feedback_set.rs line 25 at r4 (raw file):

struct FeedbackSetAlgoContext<Node: ComputeScc> {
    /// Nodes that were discovered as reachable from the current run, but possibly were not yet
    /// visited.

please elaborate about the "possibly" here - what are the cases where they were visited and still pending? (IIUC, if visited after being added to pending)

Code quote:

    /// Nodes that were discovered as reachable from the current run, but possibly were not yet
    /// visited.

crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 144 at r4 (raw file):

        vec![1, 2],
        // 1:
        vec![1, 0],

Did this fail before the fix?
I think the bug was only relevant in the case of [0, 1]. Here you first find the size-1 cycle [1], and then don't add 0 to the fset and don't break. If it was [0, 1] (before the fix) you would not add 1, add 0 and break, and skip 2, missing the size-1 cycle [2].

Code quote:

vec![1, 0],

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions


crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 144 at r4 (raw file):

Previously, yuvalsw wrote…

Did this fail before the fix?
I think the bug was only relevant in the case of [0, 1]. Here you first find the size-1 cycle [1], and then don't add 0 to the fset and don't break. If it was [0, 1] (before the fix) you would not add 1, add 0 and break, and skip 2, missing the size-1 cycle [2].

it failed only on the case starting from 0.
a similar case failed for more. (where the edges were sorted)

@orizi orizi deleted the orizi/fix-missing-feedback-set-vertices branch May 20, 2024 04:05
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.

bug: gas computation / unexpected cycle in computation issue with recursive type definition
3 participants