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

[multiplyMatrices] Update implementation to pass all tests #631

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

epsilonError
Copy link
Contributor

@epsilonError epsilonError commented Jan 12, 2025

Updates implementation and passes all tests for multiplyMatrices.

I attempted to make the smallest changes per commit to pass skipped tests. Once a test was passed, I didn't skip it again (resulting in one regression that was also fixed).

Note: To see that an error is no longer thrown by multiplying empty arrays, you'll have to comment out the

check: (..._args) => {
    return true; // Treat these tests as passed
},

block in the Incorrect Data test when running htest.

@lloydk, let me know how this looks to you. And I can also fit some refactoring into this pull request before it's merged, if needed.

I wanted to prioritize minimal changes to pass the tests which would make a future refactor easier to understand before I started it.

Copy link

netlify bot commented Jan 12, 2025

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 12bc131
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/678437d278b1a700086845c7
😎 Deploy Preview https://deploy-preview-631--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@facelessuser
Copy link
Collaborator

I think the output generally looks okay to me. The results behave like I would expect. I do find the output in the HTML a bit frustrating as it strips all the brackets (which isn't something you are doing or can control). I would expect the multiplication of [[3]] and [[4]] to be [[12]], but the output strips all of that context when it is displayed and it looks like the scalar 12. So I assume it is being calculated with the appropriate dimensions, but I would have to checkout the commit locally to be sure. But generally, I think it looks good to me.

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