-
Notifications
You must be signed in to change notification settings - Fork 14
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: ensure correct handling of mtable in LaTeX output #24
base: main
Are you sure you want to change the base?
Conversation
Although it can translate correctly, the new update code fails to pass a few tests involving the tag, which may have already been removed from the relevant web standards. So, any comment or recommended approach to fix the issue? |
Hi, @yuxuan-wei118. Thanks for your contribution! However, the issue is not clear to me. Besides the test you added, could you provide some examples of input, the expected result, and the result before the changes, highlighting the broken/missing part? I would like to understand the problem a little better. |
Hi @asnunes. Thank you for your reply! One mistranslating example is the code below.
The expected result should be that: However, the original code will translate the input into the following: Please feel free to give more comments! |
const mathml = | ||
'<math xmlns="http://www.w3.org/1998/Math/MathML"><mtable columnwidth="3em 0.05em 3em 3em 3em 3em" columnspacing="0.0em" rowspacing="0.05ex" rowlines="solid" columnlines="solid" frame="solid" framespacing="0.0em 0ex"><mtr><mtd><mi>x</mi></mtd><mtd><mspace width="0.0em" height="2.5ex" depth="1.0ex"/></mtd><mtd><mo>−</mo><mn>1</mn></mtd><mtd><mn>0</mn></mtd><mtd><mn>1</mn></mtd><mtd><mn>2</mn></mtd></mtr><mtr><mtd><mi>g</mi><mrow><mo>(</mo><mi>x</mi><mo>)</mo></mrow></mtd><mtd><mspace width="0.0em" height="2.5ex" depth="1.0ex"/></mtd><mtd><mn>6</mn></mtd><mtd><mn>4</mn></mtd><mtd><mn>2</mn></mtd><mtd><mo>−</mo><mn>1</mn></mtd></mtr><mtr><mtd><msup><mi>g</mi><mo></mo></msup><mrow><mo>(</mo><mi>x</mi><mo>)</mo></mrow></mtd><mtd><mspace width="0.0em" height="2.5ex" depth="1.0ex"/></mtd><mtd><mo>−</mo><mn>1</mn></mtd><mtd><mo>−</mo><mn>7</mn></mtd><mtd><mo>−</mo><mn>2</mn></mtd><mtd><mo>−</mo><mn>3</mn></mtd></mtr></mtable></math>'; | ||
const result = MathMLToLaTeX.convert(mathml); | ||
//console.log('result', result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead commented code
it('add begin array at the front', () => { | ||
const mathml = mathmlStrings.mtable; | ||
const result = MathMLToLaTeX.convert(mathml); | ||
console.log(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot console.log
// new method to turn string to numbers | ||
extractAndConvertNumbers(dimensionstring: string | undefined) { | ||
if (!dimensionstring) { | ||
//console.warn('dimensionString is undefined or null, default to 0.0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead console.log
.join(''); | ||
|
||
//columnwidth | ||
const columnwidth = this._mathmlElement.attributes['columnwidth']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary comment and case: columnwidth
-> columnWidth
//columnwidth | ||
const columnwidth = this._mathmlElement.attributes['columnwidth']; | ||
const widthValues = this.extractAndConvertNumbers(columnwidth); | ||
const columnwidthCm = widthValues.map((value) => value * 0.423); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TableRows
-> tableRows
// | ||
|
||
//rowlines | ||
const rowlines = this._mathmlElement.attributes['rowlines']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncessary comment
// | ||
|
||
//columnlines | ||
const columnlines = this._mathmlElement.attributes['columnlines']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncessary comment
latex = rows.join(' \\\\'); | ||
} | ||
|
||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncessary comment
// | ||
|
||
//framespacing | ||
const framespacing = this._mathmlElement.attributes['framespacing']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncessary comment
case
const spacingValues = this.extractAndConvertNumbers(framespacing); | ||
const horizontalSpacingEm = spacingValues[0] || 0; | ||
const verticalSpacingEx = spacingValues[1] || 0; | ||
let Tablerows = latex.split('\\\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @yuxuan-wei118. The feature are now clear to me, but I have some concerns that need to be addressed before merging:
- Some of the existing tests are not passing, like shown in the workflow I run and in the text below. Do you disagree with the expected result or the code should be changed to make these tests pass:
FAIL __tests__/index.test.ts
● Console
console.log
\left(\right. \begin{array}{ccc}{1 }&{ 2 }&{ 3 } \\{ 4 }&{ 5 }&{ 6 } \\{ 7 }&{ 8 }&{ 9}\end{array} \left.\right)
at Object.<anonymous> (__tests__/index.test.ts:1184:13)
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › with open attribute as [ › returns a bmatrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{bmatrix} x & y \\\\ z & w \\end{bmatrix}"
Received: "A = \\begin{bmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{bmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › with open attribute as ( › returns a pmatrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{pmatrix} x & y \\\\ z & w \\end{pmatrix}"
Received: "A = \\begin{pmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{pmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › with open attribute as | › returns a vmatrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{vmatrix} x & y \\\\ z & w \\end{vmatrix}"
Received: "A = \\begin{vmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{vmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › with open attribute as { › returns a Bmatrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{Bmatrix} x & y \\\\ z & w \\end{Bmatrix}"
Received: "A = \\begin{Bmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{Bmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › with open attribute as || › returns a Vmatrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{Vmatrix} x & y \\\\ z & w \\end{Vmatrix}"
Received: "A = \\begin{Vmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{Vmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with mtable, mtr and mtd tag › without open attribute › returns a matrix representation in latex
expect(received).toBe(expected) // Object.is equality
Expected: "A = \\begin{bmatrix} x & y \\\\ z & w \\end{bmatrix}"
Received: "A = \\begin{bmatrix} \\begin{array}{cc}{x }&{ y } \\\\{ z }&{ w}\\end{array} \\end{bmatrix}"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with partial function › returns latex partial function representation
expect(received).toBe(expected) // Object.is equality
Expected: "f \\left(x\\right) = \\left{\\begin{matrix} x^{2} , x < 0 \\\\ e^{x} , x \\geq 0 \\end{matrix}\\right"
Received: "f \\left(x\\right) = \\left{\\begin{matrix} \\begin{array}{c}{ x^{2} , x < 0 } \\\\{ e^{x} , x \\geq 0}\\end{array} \\end{matrix}\\right"
● #convert › given mfenced tag › when mfenced represents a matrix › given math string with nested mtables › adds matrix to inner table
expect(received).toBe(expected) // Object.is equality
Expected: "\\begin{bmatrix} \\begin{matrix}a_{11} & a_{12}\\end{matrix} & \\begin{matrix}\\ldots & \\ldots\\end{matrix} & a_{1 n} \\\\ \\begin{matrix}a_{21} & a_{22}\\end{matrix} & \\begin{matrix}\\ddots & \\end{matrix} & a_{2 n} \\\\ \\begin{matrix}\\begin{matrix}\\vdots & \\vdots\\end{matrix} \\\\ \\begin{matrix}a_{m 1} & a_{m 2}\\end{matrix}\\end{matrix} & \\begin{matrix}\\begin{matrix} & \\ddots\\end{matrix} \\\\ \\begin{matrix}\\ldots & \\ldots\\end{matrix}\\end{matrix} & \\begin{matrix}\\vdots \\\\ a_{m n}\\end{matrix} \\end{bmatrix}"
Received: "\\begin{bmatrix} \\begin{array}{ccc}{\\begin{array}{cc}{a_{11} }}&{{ a_{12}}\\end{array} }&{ \\begin{array}{cc}{\\ldots }}&{{ \\ldots}\\end{array} }& a_{1 n} \\\\{ \\begin{array}{cc}{a_{21} }}&{{ a_{22}}\\end{array} }&{ \\begin{array}{cc}{\\ddots }}&{{ }\\end{array} }& a_{2 n} \\\\{ \\begin{array}{c}{\\begin{array}{cc}{\\vdots }}}&{{{ \\vdots}\\end{array} } } \\\\{{ \\begin{array}{cc}{a_{m 1} }}}&{{{ a_{m 2}}\\end{array}}\\end{array} }&{ \\begin{array}{c}{\\begin{array}{cc}{ }}}&{{{ \\ddots}\\end{array} } } \\\\{{ \\begin{array}{cc}{\\ldots }}}&{{{ \\ldots}\\end{array}}\\end{array} }&{ \\begin{array}{c}{\\vdots } } \\\\{{ a_{m n}}\\end{array}}\\end{array} \\end{bmatrix}"
-
I didn't get the expected behavior for
hspace
. Is there a case where you do not addhspace
? I am concerned about adding unnecessary text to the final LaTeX output. It should be as clean as possible and only include the information present in the original MathML text. -
We also need to point out the issues in the reviewed code.
Once again, thanks for taking the time to add this PR. I will be happy to merge and publish the changes once these issues are addressed.
Hi @asnunes.
Here is an example mathml code that you can see
Please feel free to give more comments! |
The original code may mistranslate the tag. I fixed the issue by using array{ccc} to express the final output, and it should be able to translate to Latex correctly now. I also add a few tests to prove that.