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

Enhancement: Include metadata in BasicSymbolic isequal comparison #669

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bowenszhu
Copy link
Member

This pull request enhances the isequal function for BasicSymbolic to incorporate metadata in the comparison. Previously, metadata was ignored when checking for equality between two symbolic objects. This could lead to incorrect results when mathematically equivalent sysmbolic objects have different metadata, especially for #658.

This change ensures that two BasicSymbolic objects are considered equal only if they have the same expression type, the same symtypes, and identical metadata. This PR specifically supports the work done in #658 for internally handling hash collision.

@bowenszhu
Copy link
Member Author

Following up on @AayushSabharwal's point from the Oct 31 meeting: including metadata in BasicSymbolic isequal is breaking ModelingToolkit integration tests. Should we prioritize metadata compatibility or maintaining the current integration tests? Need a decision here to move forward.

@AayushSabharwal
Copy link
Contributor

Adding metadata equality to isequal would be breaking for SU and require a massive change to MTK to support. In the October 31st meeting, I was suggesting considering metadata specifically for hashconsing. Similar to the hash2 function, an isequal2 could be used to check for hash collisions. Effectively, something like:

h = hash2(sym)
cached = get(wvd, h, nothing)
if cached === nothing
    wvd[h] = sym
elseif isequal2(cached, sym)
    return cached
else
    return sym
end

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.

isequal for BasicSymbolic should consider metadata
2 participants