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

Replace recursive calls in typed-functions with this-style calls #1903

Merged
merged 4 commits into from Jul 13, 2020

Conversation

nickewing
Copy link
Contributor

As described in the issue here: #1885, if a typed function invokes itself from within a local context, as is done in many of math functions in the library, it will not have access to signatures merged in later on. The typed-function library now supports self invocation using this to address this issue.

This PR attempts to replace all the instances of recursion with the new this-style call.

@josdejong
Copy link
Owner

This PR attempts to ...

😂 not needed to be that modest, you're doing a great job Nick! Thanks a lot!

I've run the benchmarks in mathjs too, and the performance stays the same 👍 .

I think this PR currently fixes 99% of the cases, impressive. I noticed the following edge cases:

  1. The following functions can still be refactored to use this for self references: boolean, string, bignumber, complex, fraction, unit. (To find them, I did a search in the project for all cases of = typed().

  2. There are a number of cases where we still use variables like:

    var transpose = typed(...)
    // ...
    return transpose

    and not do directly return typed(...). The cases I saw are: derivative, rationalize, apply, eigs, transpose, gamma, erf, intersect, multiply, sqrtm. To make this work we can either utilize the fact that JavaScript has function hoisting and simply replace the variable definition with return in most of these cases, or move the typed function to the bottom of the function. I think the latter makes most sense. Not having the function defined as a variable prevents accidentally using it, so I think it's good to try to remove it as much as possible.

  3. This function needs a more thorough refactor to be able to utilize the new self referencing: compareNatural

If you want I can take a look at these edge cases too.

@nickewing
Copy link
Contributor Author

I replaced the remaining assignments of typed-functions. I'm not sure if you had anything in particular in mind for the refactor of compareNatural, so let me know if you have any thoughts there.

@josdejong
Copy link
Owner

Thanks Nick!

About the compareNatural: the self-reference is used in the helper functions compareArrays and compareObjects only. I think we can simply pass a new, third argument compareNatural to these functions.

And for functions like derivative I think it's most clear to move the typed function to the bottom, so we have the `return typed('derivative', ...)' at the bottom which looks most natural.

@nickewing
Copy link
Contributor Author

I actually already refactored compareNatural but I wasn't sure if that was what you had in mind. I think we're probably on the same page there.

The complication with derivative and simplify is that they both assign additional properties to the the typed function. Without really digging in there, it's not clear to me what the purpose of some of those properties is.

From derivative:

  derivative._simplify = true

  derivative.toTex = function (deriv) {
    return _derivTex.apply(null, deriv.args)
  }

From simplify:

  simplify.simplifyCore = simplifyCore
  simplify.resolve = resolve

I see the note about moving toTex into latex.js which makes sense, but what we should do with the others as clear.

@josdejong
Copy link
Owner

Hm, you're right, there are properties attached in derivative and simplify. I vaguely remember there was a difficulity that but would have to dive into it to figure it out again. I think though that it goes to far for this PR and we can better keep that for a second round, it goes a bit too far "off topic".

I now see you've indeed already refactored compareNatural, thanks!

So, I think this PR is ready to merge, right?

@nickewing
Copy link
Contributor Author

Yeah, I think we're probably good to go.

@josdejong
Copy link
Owner

Yeah, I think we're probably good to go.

🎉 will merge now and publish a new version tonight. Thanks again Nick for all your effort!

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.

None yet

2 participants