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: tdim for other logic operators #1341

Merged
merged 7 commits into from
Feb 29, 2024

Conversation

JulienBalianSonos
Copy link
Contributor

  • support TDim for others cmp operators

@kali
Copy link
Collaborator

kali commented Feb 28, 2024

I am not sure this is going to do what we want on the long run. It will compare TDim based on... alphabetic order of symbols, or numbers before symbols and the like. My inclination would be:

  • remove Ord and PartialOrd on TDim altogether
  • implement comparison by casting to integer first during expansion, adding an actual Cast to i64 node in the net before getting into the comparison.
  • fix whatever else is broken, including the Einsum/MatMul heuristics.

WDYT ?

@JulienBalianSonos
Copy link
Contributor Author

JulienBalianSonos commented Feb 28, 2024

Indeed that seems a better way to handle what we really want. I will update this PR accordingly.

@kali kali merged commit 0031df4 into sonos:main Feb 29, 2024
45 checks passed
@JulienBalianSonos JulienBalianSonos deleted the fix/tdim-bool-cmp branch February 29, 2024 17:56
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.

2 participants