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

Fix lambda theory test #236

Merged
merged 10 commits into from
Aug 23, 2024

Conversation

gkronber
Copy link
Collaborator

PR for #235.
Fixes semantic analysis code for lambda theory test case and a bug which caused missing updates of parent eclasses after the semantic value of an eclass was changed.

Copy link
Collaborator

@olynch olynch left a comment

Choose a reason for hiding this comment

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

@0x0f0f0f for what it's worth, I read through this bug fix and it seems correct to me, at least with respect to my understanding as captured in this comment:

"""
merge_analysis_data!(a::EClass{D}, b::EClass{D})::Tuple{Bool,Bool,Union{D,Nothing}} where {D}
Returns a tuple of: `(did_update_a, did_update_b, newdata)` where:
- `did_update_a` is a boolean indicating whether `a`'s analysis class was updated
- `did_update_b` is a boolean indicating whether `b`'s analysis class was updated
- `newdata` is the merged analysis data
"""
.

In retrospect, if I had paid more attention while writing that comment, I could have caught this...

@gkronber
Copy link
Collaborator Author

Let me add a few more test assertions to check the free variable analysis before merging this.

src/EGraphs/egraph.jl Outdated Show resolved Hide resolved
src/EGraphs/saturation.jl Outdated Show resolved Hide resolved
@0x0f0f0f
Copy link
Member

Nice, can you trigger CI here as well pleae?

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.06597% with 149 lines in your changes missing coverage. Please review.

Please upload report for BASE (ale/3.0@c43b0fb). Learn more about missing BASE report.

Files Patch % Lines
src/EGraphs/Schedulers.jl 43.47% 26 Missing ⚠️
src/utils.jl 7.69% 24 Missing ⚠️
src/EGraphs/egraph.jl 87.29% 23 Missing ⚠️
src/Syntax.jl 88.66% 17 Missing ⚠️
src/Patterns.jl 74.46% 12 Missing ⚠️
src/EGraphs/saturation.jl 95.20% 7 Missing ⚠️
src/Rules.jl 83.33% 7 Missing ⚠️
ext/Plotting.jl 0.00% 6 Missing ⚠️
src/Rewriters.jl 53.84% 6 Missing ⚠️
src/ematch_compiler.jl 95.83% 6 Missing ⚠️
... and 6 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             ale/3.0     #236   +/-   ##
==========================================
  Coverage           ?   80.78%           
==========================================
  Files              ?       19           
  Lines              ?     1504           
  Branches           ?        0           
==========================================
  Hits               ?     1215           
  Misses             ?      289           
  Partials           ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@0x0f0f0f
Copy link
Member

@gkronber ok to merge?

@0x0f0f0f 0x0f0f0f merged commit 1dc53da into JuliaSymbolics:ale/3.0 Aug 23, 2024
3 of 4 checks passed
@gkronber gkronber deleted the fix_lambda_theory_test branch September 3, 2024 14:12
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.

4 participants