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.

Sign up for GitHub

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

Jump to bottom

#204 fix: BFS implementation #208

Merged
merged 2 commits into from
Oct 28, 2024
Merged

#204 fix: BFS implementation #208

merged 2 commits into from
Oct 28, 2024

Conversation

trungnotchung
Copy link
Contributor

@trungnotchung trungnotchung commented Oct 28, 2024 •
edited
Loading

Refactors the areCausallyRelatedUsingBFS method to simplify the BFS implementation for checking causal relationships between vertices.

@d-roak d-roak requested review from JanLewDev and h3lio5 October 28, 2024 09:00
d-roak
d-roak requested changes Oct 28, 2024
View reviewed changes
Copy link
Member

@d-roak d-roak left a comment

Choose a reason for hiding this comment

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

rm useless comments and rename the fns
try to build the code, it might need some refactors because the naming would be different

packages/object/src/hashgraph/index.ts Outdated Show resolved Hide resolved
packages/object/src/hashgraph/index.ts Show resolved Hide resolved
JanLewDev
JanLewDev approved these changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@JanLewDev JanLewDev left a comment

Choose a reason for hiding this comment

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

Clean logic, I like it. I'm slightly confused with the naming as I think the bitset version should be default and BFS version used when memory consumption can become a problem

@d-roak d-roak force-pushed the bugfix/incorrect-bfs-implementation branch from 19abae7 to 1338a99 Compare October 28, 2024 14:22
d-roak
d-roak approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@d-roak d-roak left a comment

Choose a reason for hiding this comment

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

lgtm

@d-roak d-roak merged commit 81b7de2 into main Oct 28, 2024
7 checks passed
@d-roak d-roak deleted the bugfix/incorrect-bfs-implementation branch October 29, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanLewDev JanLewDev JanLewDev approved these changes

@d-roak d-roak d-roak approved these changes

@h3lio5 h3lio5 Awaiting requested review from h3lio5

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trungnotchung @d-roak @JanLewDev