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

Wrong behavior around multiplication #89

Closed
odashi opened this issue Nov 12, 2022 · 13 comments · Fixed by #139 or #182
Closed

Wrong behavior around multiplication #89

odashi opened this issue Nov 12, 2022 · 13 comments · Fixed by #139 or #182
Assignees
Milestone

Comments

@odashi
Copy link
Collaborator

odashi commented Nov 12, 2022

The current implementation converts a * b into $ab$. This only works if the variables have only 1 character and doesn't generate a correct syntax for other cases:

  • Long names: abc * def --> $abcdef$, which is hard to distinguish from other combination of variables.
  • Unary operators: x * -y --> $x - y$, which is definitely wrong.

These cases require explicit multiply operator ( $\cdot$ or $\times$ ) between the operands.

I think the Mult(left, right) subtree should be converted to $left \cdot right$ by default, and try to avoid the operator only in the following cases:

  • The left-hand side operand is a single character or a bracket (with or without unary operators and/or super/subscripts)
  • The right-hand side operand is a single character or a bracket (with or without super/subscripts, without unary operators)

This requires fine-graind name processing, and may be applied after #82.

@odashi
Copy link
Collaborator Author

odashi commented Nov 20, 2022

Anyway, I will handle this issue today.

@odashi
Copy link
Collaborator Author

odashi commented Nov 20, 2022

Anyway some simple heuristic can be applied without waiting for #82

@odashi
Copy link
Collaborator Author

odashi commented Dec 9, 2022

Re-open this issue since it is actually not resolved yet.

@odashi odashi reopened this Dec 9, 2022
@ZibingZhang
Copy link
Contributor

Mind if I take a look into this?

@odashi
Copy link
Collaborator Author

odashi commented Dec 9, 2022

Maybe it's good if we get a list of possible LHS-RHS pairs with/without the multiplication symbol, e.g.,

left right tex
3 x $3 x$
x y $x \cdot y$

@ZibingZhang
Copy link
Contributor

I can't think of a case where we would want to remove the \cdot other than the case of constant * name.

left right tex why
3 x $3x$ basic multiplication with constant (should work with floats and complex)
x 3 $x \cdot 3$ should not reorder for user, if they want lack of \cdot, can change order themselves
two names $two \cdot names$ two variables should always have a \cdot
3 x * 4 * y $3x \cdot 4y$ should work in more complicated expression as well

@odashi
Copy link
Collaborator Author

odashi commented Dec 10, 2022

Some complete examples:

numeric alphabet math word function bracket
numeric $2 \cdot 3$ $\color{red} 2 y$ $\color{red} 2 \beta$ $\color{red} 2 \mathrm{bar}$ $\color{red} 2 g(y)$ $\color{red} 2 (u + v)$
alphabet $x \cdot 3$ $\color{red} x y$ $\color{red} x \beta$ $x \cdot \mathrm{bar}$ $x \cdot g(y)$ $x \cdot (u + v)$
math $\alpha \cdot 3$ $\color{red} \alpha y$ $\color{red} \alpha \beta$ $\alpha \cdot \mathrm{bar}$ $\alpha \cdot g(y)$ $\alpha \cdot (u + v)$
word $\mathrm{foo} \cdot 3$ $\mathrm{foo} \cdot y$ $\mathrm{foo} \cdot \beta$ $\mathrm{foo} \cdot \mathrm{bar}$ $\mathrm{foo} \cdot g(y)$ $\mathrm{foo} \cdot (u + v)$
function $f(x) \cdot 3$ $f(x) \cdot y$ $f(x) \cdot \beta$ $f(x) \cdot \mathrm{bar}$ $f(x) \cdot g(y)$ $f(x) \cdot (u + v)$
bracket $(s + t) \cdot 3$ $\color{red} (s + t) y$ $\color{red} (s + t) \beta$ $\color{red} (s + t) \mathrm{bar}$ $\color{red} (s + t) g(y)$ $\color{red} (s + t) (u + v)$

In cases {alphabet,math,word}-{bracket} it would make some confusion between them and function calls if we removed \cdot.

EDIT(2023-10-16)

Added some consistent rules:

  1. Numeric should always keep the preceding \cdot.
  2. Functions should always keep the following \cdot due to some ambiguity (e.g., sin(x) * y -> $\sin xy$)
  3. Numeric and brackets can remove the following \cdot without ambiguity, except the next operand is numeric.

@ZibingZhang
Copy link
Contributor

ZibingZhang commented Dec 10, 2022

I think alphabet-alphabet would also cause confusion, because you could have a (arguably dumb) case like

def (x, y, xy):
    return xy

In general though when user sees $xy$ there's no easy way of telling if it's $x \cdot y$ or a single variable $xy$, so I think it we should keep the \cdot.

edit: I'm realizing that word is stylized differently than alphabet, so it should be clear that $\mathrm{xy}$ is different than $xy$.

@odashi
Copy link
Collaborator Author

odashi commented Dec 10, 2022

I'm realizing that word is stylized differently than alphabet

Yes this is intended in #139, but maybe the result is somewhat confusing for some users.

@ZibingZhang
Copy link
Contributor

ZibingZhang commented Dec 11, 2022

Looking through the code this seems like we'll have to utilize information from expr codegen to figure out what nodes are wrapped in parens , as well as information from identifier replacer to differentiate between math vs. alphabet vs. word.

@ZibingZhang
Copy link
Contributor

Another situation, if we have a * b @ x @ y, this should result in $ab \cdot xy$.

@odashi
Copy link
Collaborator Author

odashi commented Dec 11, 2022

Another situation, if we have a * b @ x @ y, this should result in .

It looks $abxy$ is acceptable. $ab \cdot xy$ looks to represent (a*b)@(x@y), but:

  • Matrix multiplication is an associative operation: position of parenthesis is meaningless
  • We get the AST (((a*b)@x)@y) for this expression.

@odashi
Copy link
Collaborator Author

odashi commented Dec 11, 2022

I think we eventually need more informative object than the raw string for expression codegen to implement this feature (and other stuff that rely on the LaTeX syntax rules). let me implement it later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants