-
Notifications
You must be signed in to change notification settings - Fork 107
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
Matrix Library Pull Request #1599
base: horizon
Are you sure you want to change the base?
Conversation
…decimal compatability
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.
This is not ready yet. There are lots of missing or wrong annotations to be checked, incorrect logic in several places, some dubious stack-safety issues to be addressed, and more tests are definitely needed (especially since some functions haven't been tested at all). My comments below are a starting point for you; there are likely more issues to be addressed. When you're dealing with annotations, stack safety, equality, etc, you have to cultivate a certain kind of "skeptical paranoia", as in, "What could a clueless or malicious user do, that would violate some assumption I made and cause my code to break? What could an unfortunate scheduling in the browser do to force my code to pause at an inconvenient point?" I'm happy to set up a meeting time to go over the code with you and review what needs fixing, if you'd like, but I do want you to go through the feedback below and try to categorize it into big clumps of related comments (e.g. "annotations missing or wrong", "tests missing or incomplete", "stack safety wrong", etc), and then go back through your code function-by-function and see which categories apply to each function.
Is there documentation to go with any of this code yet, or not yet?
return runtime.safeCall( | ||
function(){return recEq.app(self.$underlyingMat[i],other.$underlyingMat[i])}, | ||
function(result){ | ||
if (runtime.ffi.isNotEqual(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.
This logic is wrong. Depending on the order you have values in the matrix, you could have a roughnum followed by an exactnum. Consider [mat(2, 2): 1, 2, ~3, 4]
and [mat(2, 2): 1, 2, ~5, 4]
. The roughnums will compare as Unknown
, which you ignore, and then the recursive call that compares 4 will say they're Equal
, and you'll return that as the answer. Alternately, if the last values happened to not be equal, then you'd return NotEqual
which would happen to be correct here. And finally, if the values happened to have the roughnums last, then you'd return Unknown
which would happen to be correct. You need to use the logic of equal-and
to combine your answers here. And you likely want to use raw_array_fold
instead of rolling your own loop.
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.
Resolved. I'm not using raw_array_fold because we want to terminate early if not equal.
I've addressed all comments and increased test coverage. There are still 2 issues :
Callin |
Addressed both comments and made similar fix in |
Hi,
As was requested by the core Pyret team, William Sun and I wrote this library containing core linear algebra features.
What The Library Does :
What The Library Does Not Do :
Setup
[mat(2,2):12,4,5,6]
and a vector by[vector(3):1,3,-42]
Naming Conventions
func-mat
such asrref-mat
. However some functions are given by just their name.Phillip Blair's Library
A few years ago Phillip Blair wrote a matrix library (https://github.com/brownplt/pyret-lang/blob/8bf1d79c283d50a04daae1d3e4a7682e606c1c02/src/arr/trove/matrices.arr). Ours is equivalent , the critical difference being that it is Javascript rather than Pyret. You can find the modern version of Phillip's library compliant with 2021 (https://code.pyret.org/editor#share=1_UVB6S9ad3F9KRcrdNx3yrUyAw-rGdfo&v=1599623)
This makes a huge performance difference. The Pyret version times out for matrices as small as 50x50 (takes > 5mins) whereas the Javascript version (ours) does not show any lag.
Issues To Address Before Adding
build-mat
usesafeCall
s to avoid blowing up the stack. However Pyret decides at some time to just halt the computation. So :I'm unsure as to how to solve this while still keeping these functions safe.
Other Things To Address
Anirudh Narsipur