Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Added Identity Function for Math lib #37

Merged
merged 5 commits into from
May 24, 2022

Conversation

MudroadWhite
Copy link
Contributor

Identity Function:

identity(n) (creates 2-d identity matrix with nxn size)

Copy link
Member

@lidangzzz lidangzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi here are my reviews. Let me know if you have any other questions. Thx.

identity.hhs Outdated Show resolved Hide resolved
identity.hhs Outdated Show resolved Hide resolved
identity.hhs Outdated Show resolved Hide resolved
identity.hhs Show resolved Hide resolved
identity.hhs Show resolved Hide resolved
@Gaoooooo
Copy link
Collaborator

Everything after these changes will look perfect. I think your way is faster than the mathjs way, as you dont have to copy twice and dont have to convert to Mat and back nor make a call to mathjs library. Will merge after these small issues are fixed - thank you!

Copy link
Collaborator

@Gaoooooo Gaoooooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i accidentally left these in pending changes

identity.hhs Outdated
return new Mat([[1]]);
}

// Now we can start on creating the Id matrix
let result = [];
let result = new Mat().zeros(n, n);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let result = new Mat().zeros(n, n);
let result = new Mat().zeros(input, input);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n not defined, i assume you meant input for this and the one below (for loops n -> input)

identity.hhs Outdated
return new Mat([[1]]);
}

// Now we can start on creating the Id matrix
let result = [];
let result = new Mat().zeros(n, n);
for (let i = 0; i < n; i++){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (let i = 0; i < n; i++){
for (let i = 0; i < input; i++){

identity.hhs Outdated
row[i] = 1;
result += row;
}
result[i][i] = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result[i][i] = 1;
result.val[i][i] = 1;

because we're modifying a Mat instead of a JS array we can't access the indices directly, but rather have to access them through .val otherwise we get a 'cannot read 0' error

@@ -16,23 +15,27 @@ function identity(input) {
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( !(arguments.length === 1) ) {
throw new Error('Exception occurred in identity. Only 1 argument allowed, but a different number were entered.');
}

just a good safety check on the arguments so if the user uses the function incorrectly, they know which function they used incorrectly and why

@Gaoooooo Gaoooooo merged commit f00ba65 into Hedgehog-Computing:main May 24, 2022
@Gaoooooo
Copy link
Collaborator

Good job, I apologize for the late merge, I thought I merged it earlier.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants