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

Incorrect LaTeX for some predefined functions when passing the wrong number of parameters #636

Closed
FSMaxB opened this Issue Apr 3, 2016 · 14 comments

Comments

Projects
None yet
2 participants
@FSMaxB
Collaborator

FSMaxB commented Apr 3, 2016

I just noticed a bug in the LaTeX output. Let's say you parse sin(a,b) = a+b; sin(1, 2) and run toTex() on it, the result is '\\mathrm{sin}\\left(x,\\mathrm{y}\\right):= x+ y;\\;\\;\n\\sin\\left(1\\right)'. The last occurence of sin misses the second parameter.

This happens because sin provides a template for LaTeX generation that only uses one parameter: '\\sin\\left\\(${args[0]\\}\\right)'.

I have no idea how to fix this without doing one of the following:

  • keeping track of redefined functions and disabling the LaTeX-Templates for them (ugly)
  • change every single function that supplies it's own LaTeX-String to support all numbers of arguments. (much work + ugly)
  • Let the parser modify the .toTex properties of functions once they are redefined (can't do this because of it's side effects on the global mathjs instance)

Or we could just ignore this issue and hope that no one actually redefines one of the functions that provide a custom LaTeX output and still expects it to behave correctly with toTex().

@FSMaxB FSMaxB added the bug label Apr 3, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 5, 2016

wow, that's a tricky one. I doubt whether this would ever occur in practice but it is not impossible.

I think that your second option is best: change the LaTeX template of all functions to display all arguments ('\\sin\\left(${args}\\right)') instead just one or two. I don't think this is a particularly ugly solution, what do you find ugly about it?

It would be a relatively simple refactoring though we need to be careful to replace only the right occurrences. I can do the refactoring, this is easy to do with WebStorm.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 5, 2016

It's ugly in two regards:

  1. It needs to be done for every single function that defines it's own LaTeX output, even ones that will be added in the future.
  2. It makes the template language I introduced quite pointless for most functions, because you would suddenly need to distinguish between different amounts of arguments thereby requiring a dedicated handler function that handles those cases separately.

UPDATE:
3. Many of those functions that have their own LaTeX output defined aren't as simple cases as sin. See combinations for example. (this relates to point 2!)

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 7, 2016

Thanks for your clear arguments. Yes for functions we will have to use ${args} instead of ${args[0]}, but I don't see a problem there. What I find more important is a solution which does not silently hide information from you, even if the information is partly invalid like sin(1,2,3). It's up to the user to fix that but if you hide this from the user, he has no way to know about it and fix it.

The template language is an will be essential for all operators and some special functions like nthRoot and combinations. But we should not use the template stuff for the sake of using it but where it's appropriate and needed.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 8, 2016

I don't understand what you mean by "hiding information".

But reading the code I just noticed, that the solution to the problem I described doesn't actually require a handler function. It should be as easy doing this for all functions (all other numbers of arguments will automatically fall back to the default template):

sin.toTex = {
  // LaTeX-Template for 1 argument
 1: '\\sin\\left\\(${args[0]\\}\\right)';
}

See FunctionNode line 354.

I didn't even remember that I wrote this, but now that I see it, I kind of remember using it for the LaTeX output of some of the functions, not sure which though.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 8, 2016

I'm not sure why this isn't documented. We'd have to take a look at the discussion we had at the time. My guess is that it shouldn't be considered part of the API so it wasn't documented, but I'm not sure.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 8, 2016

Ah, indeed :). I don't know why it's wasn't documented.

With "hiding information" I just mean that if you input say sin(1,2,3), and call toTex, the tex representation shows sin(1) and hides the last two arguments from you, which can give confusion since these two arguments are actually there.

So there are two options to replace the current templates:

sin.toTex = '\\sin\\left\\(${args[0]\\}\\right)';
  • option 1

    sin.toTex = {
     '1': '\\sin\\left\\(${args[0]\\}\\right)';
    }
  • option 2

    sin.toTex = '\\sin\\left\\(${args\\}\\right)';

Option 1 is most correct, but it contains a bit more magic. I have a slight preference for option 2 since that's simpler to understand and we're talking about an edge case only. I don't have a strong opinion here. What has your preference?

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 8, 2016

I strongly prefer option 1 because most LaTeX templates that were defined manually don't make too much sense if you extend them to support a variable number of arguments and users will probably expect their overwritten functions to have the default LaTeX output.

Also many of the templates can't even be extended to support variable numbers of arguments because the LaTeX output would simply break or make no sense at all. Like '\\left(${args[0]}' + latex.operators['xor'] + '${args[1]}\\right)' or '\\binom{${args[0]}}{${args[1]}}'.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 8, 2016

Ok lets go for option 1 then :)

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 8, 2016

I won't have the time to do this refactoring in the near future. You would either have to do it yourself or you'd have to wait a while until I find the time (which might be ok since this bug doesn't seem to critical).

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 8, 2016

I think I can do it, maybe this weekend. It's indeed not critical.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 11, 2016

I've gone through all functions and made the toTex templates more strict, reckoning with the number of arguments.

I've left the functions which had the default template ('\\mathrm{${name}}\\left(${args}\\right)') as they are, since some of them accept different numbers of arguments and it doesn't really help to make the toTex conditional since it falls back to the default template anyway.

Just thinking, maybe we should change the functions that use the default template:

format.toTex = '\\mathrm{${name}}\\left(${args}\\right)';

to something like:

format.toTex = undefined;  // use default template

That way you get information that this function is using the default template, and it probably saves a few bytes of the bundle.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Apr 11, 2016

Will this get stripped during the minification? Otherwise maybe remove the line entirely.

But I like the idea of explicitly stating that the default template was used. Thereby it is possible to distinguish between cases where a proper toTex is still missing and cases where the default template was intended.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 15, 2016

Thereby it is possible to distinguish between cases where a proper toTex is still missing and cases where the default template was intended.

yes exactly, and we shouldn't loose that.

This shouldn't be stripped when minifying: the minifier cannot just decide to drop a property of an object because it's (initial) value is undefined. That would alter the behavior of for example Object.keys(obj).

Ok I will refactor the places where the default toTex is used to format.toTex = undefined.

josdejong added a commit that referenced this issue Apr 15, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 15, 2016

Done in b2066e5

@josdejong josdejong closed this in 8068f4a Apr 16, 2016

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