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

Add usolveAll and lsolveAll methods #1916

Merged
merged 12 commits into from Aug 29, 2020
Merged

Conversation

m93a
Copy link
Sponsor Collaborator

@m93a m93a commented Jul 14, 2020

Fixes #1748, needed for #1743

In this pull request, I implement an algorithm that generalizes front- and back-substitution. Similar to usolve and lsolve, usolveAll and lsolveAll return an array containing the vectors that make up an affine basis of the solution space. This means that all valid solutions can be expressed as an affine combination of these vectors.

Example usage:

const M = [[0,1,2],[0,1,1],[0,0,0]]
const b = [2,2,0]
math.usolveAll(M, b)
// [ [[0], [2], [0]], [[1], [2], [0]] ]

Previously, u-, l- and lusolve thrown an error when no solution existed. I kept it this way for now, but I'm tempted to return an empty array instead, as there is nothing bad or wrong about the case when a system doesn't have a solution, and throwing an unnecessary error might force users to write a lot of surplus try-catch blocks.

@josdejong
Copy link
Owner

@josdejong josdejong commented Jul 18, 2020

Sounds good, thanks Michal! I'll review your work soon.

Makes sense to not throw an error but return an empty array when there are no solutions.

Thinking aloud here: we could also think about introducing two versions of the functions like usolve: one returning a single solution (the old behavior), and one returning all solutions. Or pass an optional flag as third argument to specify whether you want one or all solutions back.

@josdejong
Copy link
Owner

@josdejong josdejong commented Jul 19, 2020

The code looks very neat Michal, nice job 👍 . There are indeed still extra tests needed to test the new functionality.

I'm curious to hear your ideas about either changing the behavior of usolve, lsolve and lusolve, or introducing new functions or options somehow in a backward compatible way. I'm not sure if there are still use cases for the old behavior or not.

@m93a
Copy link
Sponsor Collaborator Author

@m93a m93a commented Jul 19, 2020

Initially I thought there's no advantage in only finding one solution. But when I think about it again, for very large matrices with low rank, the difference in both computation time and memory usage would be noticable. So you're right, it would be probably best to expose methods for both "find one solution" and "find all solutions".

About the API: I'm strongly for creating a new method instead of making one method with options. A robust API should only have few return types for a method :)

Later today, I'll revert the changes to usolve, lsolve and lusolve and make new methods usolveAll and lsolveAll.

@josdejong
Copy link
Owner

@josdejong josdejong commented Jul 19, 2020

Makes sense that there will be a performance hit for finding all vs just one solution.

About the API: I'm strongly for creating a new method instead of making one method with options. A robust API should only have few return types for a method :)

Later today, I'll revert the changes to usolve, lsolve and lusolve and make new methods usolveAll and lsolveAll.

Sounds like a good plan to keep the methods separate 👍 . The names usolveAll and lsolveAll are very clear, and if we reference ("See also: ..." in the comments) it should be clear that these methods exist.

@m93a
Copy link
Sponsor Collaborator Author

@m93a m93a commented Jul 19, 2020

Haha I did that, but in the meantime something broke. I tried pulling your commit with git pull --rebase but now every test fails.

Could you please help me out with this one? Prior to pulling, all the unit tests were successful.

.

Screen record from 2020-07-19 23 40 33
^^ Mood

EDIT: I have a backup. Before merging I pushed to patch-usolve3. If we don't manage to fix it, I'll make a new PR from that branch

EDIT 2: Haha the tests were failing just locally because I didn't run npm install after pulling. Disregard this post, then 😅️

EDIT 3: How do I fix the one error in Travis?

@m93a m93a changed the title Improve usolve, lsolve and lusolve to find all solutions of a system Add usolveAll and lsolveAll methods Jul 19, 2020
@josdejong
Copy link
Owner

@josdejong josdejong commented Jul 22, 2020

😂 Good to hear you figured it out.

EDIT 3: How do I fix the one error in Travis?

The new function should be added to the following index file so it will be bundled: https://github.com/josdejong/mathjs/blob/develop/src/factoriesAny.js

@m93a
Copy link
Sponsor Collaborator Author

@m93a m93a commented Jul 25, 2020

I have done that already: https://github.com/josdejong/mathjs/pull/1916/files#diff-97a500a8c25160352eb28525e7d37613
The problem that causes Travis to fail is that I haven't generated docs for the new functions. And I don't know how to…

@josdejong
Copy link
Owner

@josdejong josdejong commented Jul 25, 2020

Ahh, sorry. There is indeed also the embedded docs. Those are located in the folder:

https://github.com/josdejong/mathjs/tree/develop/src/expression/embeddedDocs/function/algebra

And the index file of the embedded docs is:

https://github.com/josdejong/mathjs/blob/develop/src/expression/embeddedDocs/embeddedDocs.js

I should make those error messages more explanatory, sorry.

@josdejong
Copy link
Owner

@josdejong josdejong commented Aug 19, 2020

@m93a did you see my latest comment? Want me to fix the embedded docs?

@m93a
Copy link
Sponsor Collaborator Author

@m93a m93a commented Aug 25, 2020

Sorry, I didn't have time to reply earlier. All set and ready to merge, sir! 😁️

@josdejong
Copy link
Owner

@josdejong josdejong commented Aug 29, 2020

Sorry, I didn't have time to reply earlier.

No problem at all! I have the same issue regularly 😄

Thanks for the updates, will merge your work now.

@josdejong josdejong merged commit ba4ff2f into josdejong:develop Aug 29, 2020
4 checks passed
@josdejong
Copy link
Owner

@josdejong josdejong commented Sep 26, 2020

Published now in v7.3.0 🎉

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