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

Expression level rewriting #69

Merged
merged 43 commits into from
Oct 3, 2024
Merged

Expression level rewriting #69

merged 43 commits into from
Oct 3, 2024

Conversation

GeorgeR227
Copy link
Contributor

This PR is meant to create a back-and-forth between ACSets, which excel at information propagation (typing, overloading, etc.) and Symbolics, which excel at expression rewriting (open_operators, optimizations, etc.).

This should remove the need for our graph rewriting functions and make the workflow to add new rewrites into the system much more faster/cleaner.

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (0a314a8) to head (67079cb).

Files with missing lines Patch % Lines
src/acset2symbolic.jl 91.83% 4 Missing ⚠️
src/deca/ThDEC.jl 55.55% 4 Missing ⚠️
src/graph_traversal.jl 94.28% 2 Missing ⚠️
src/sym_rewrite.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           symbolicutilsinterop      #69      +/-   ##
========================================================
+ Coverage                 74.90%   82.01%   +7.10%     
========================================================
  Files                        16       19       +3     
  Lines                      1068     1173     +105     
========================================================
+ Hits                        800      962     +162     
+ Misses                      268      211      -57     
Flag Coverage Δ
82.01% <90.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@jpfairbanks jpfairbanks left a comment

Choose a reason for hiding this comment

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

The topo sort stuff is looking good. The symbolics integration should target the existing symbolics integration branch.

struct TraversalNode{T}
index::Int
name::T
end
Copy link
Member

Choose a reason for hiding this comment

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

My aesthetic sense says these should be in the opposite order because of most significant digit first order. An alternative would be to use MLStyle to create a type that represents the valid elements of an ACSet. When you traverse the ACSet you create an iterable of these types. Then when you consume this list, you use @match to consume the elements of the iterable. These @data records could be automatically generated in ACSets.jl from the schema.

module ACSetElements
using MLStyle

abstract type ACSetElement end

@data PodeElement <: ACSetElement begin
  Op1(i::Int)
  Op2(i::Int)
  Var(i::Int)
  TVar(i::Int)
  Σ(i::Int)
  Summand(i::Int)
  Name(s::Symbol)
  _Type(t) # you can't use Type in ML style because Core.Type 
end
end

module ACSetRecords
using MLStyle

abstract type ACSetRecord end
@data PodeRecord <: ACSetRecord begin
  Name(s::Symbol)
  Operator(s::Symbol)
  _Type(t)
  Var(name::Name, type::Type)
  TVar(incl::Var)
  Op1(src::Var, tgt::Var, op1::Operator)
  Op2(proj1::Var, proj2::Var, res::Var, op2::Operator)
  Σ(sum::Var)
  Summand(summation:, summand::Var)
end
end

Copy link
Member

Choose a reason for hiding this comment

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

Ok, the unitype design that you did, coupled with an Enum that captures the available symbols would be good. https://docs.julialang.org/en/v1/base/base/#Base.Enums.@enum and https://thautwarm.github.io/MLStyle.jl/latest/syntax/pattern.html#support-pattern-matching-for-julia-enums could work together to make it easy to write the generic code.

src/graph_traversal.jl Outdated Show resolved Hide resolved
src/graph_traversal.jl Outdated Show resolved Hide resolved

function is_correct_length(d::SummationDecapode, result)
return length(result) == number_of_ops(d)
end
Copy link
Member

Choose a reason for hiding this comment

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

We can also check that when you iterate over the edges in the decapode, we never go from a later vertex to an earlier vertex.

@lukem12345
Copy link
Member

When roundtripping to the SummationDecapode is complete, it would be good to test that the results in test/openoperators.jl give the same results up to renaming etc

@GeorgeR227 GeorgeR227 changed the base branch from main to symbolicutilsinterop August 30, 2024 18:35
@GeorgeR227
Copy link
Contributor Author

I've rebased this branch to work off of PR #64 since the code here will be used for the rewriting using only Sort.

algebraicjuliabot and others added 12 commits September 18, 2024 13:30
Added `apex` and `@relation`, `to_graphviz` from Catlab

Co-authored-by: James <[email protected]>
Converts ACSet to a series of Symbolic terms that can be rewritten with a provided rewriter
Added a short script showcasing how rewriting could be done with the `Sort` types and a reference ACSet.
This now supports the ability for ACSet intermediate expressions to be merged into one single expression upon which rewriting rules (like dd=0) may be performed.
Can take ACSets to Symbolics back to  ACSets
This needs to switch to use the new type system
Also switched to using SymbolicsUtils' `substitute`. Still needs tests and code needs to be cleaned up.
GeorgeR227 and others added 9 commits September 18, 2024 13:42
Addition now works as well but rewriting seems to be janky, unrelated to this pipeline specifically I believe.
This black boxes the intermediate symbolic expressions to the user. The user will simply submit a rewriter that will then be applied
src/sym_rewrite.jl Outdated Show resolved Hide resolved
@lukem12345
Copy link
Member

lukem12345 commented Sep 27, 2024

@quffaro It looks like your commit 2b3198f removed the check for $$\partial_t$$ in extract_symexprs, but your merge 367414d seems to have possibly errantly put the check back. Which behavior do you want. Is there a test that passes or fails depending on whether this is checked?

Remove special DerivOp handling, fixed bug where multiple equations with the same variable result were being dropped, more tests to cover these cases and further clean up.
@GeorgeR227
Copy link
Contributor Author

@lukem12345 We wanted to remove this check since it lead to a lot of special code to handle the final ACSet generation. I've removed it in commit 5b84cc8 and added the appropriate tests to ensure it's proper functionality.

src/acset2symbolic.jl Outdated Show resolved Hide resolved
test/decasymbolic.jl Outdated Show resolved Hide resolved
src/acset2symbolic.jl Outdated Show resolved Hide resolved
@GeorgeR227 GeorgeR227 marked this pull request as ready for review October 2, 2024 18:33
@quffaro quffaro merged commit 5d5c25d into symbolicutilsinterop Oct 3, 2024
7 of 8 checks passed
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.

5 participants