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

feat: add math/base/special/lcmf #3096

Open
wants to merge 43 commits into
base: develop
Choose a base branch
from

Conversation

Harsh-Mathur-1503
Copy link

@Harsh-Mathur-1503 Harsh-Mathur-1503 commented Nov 11, 2024

Resolvesa part of #649.

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Open to reviews.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Nov 11, 2024
@Harsh-Mathur-1503 Harsh-Mathur-1503 marked this pull request as ready for review November 11, 2024 11:23
@Harsh-Mathur-1503
Copy link
Author

@gunjjoshi I've worked on this package lcmf
Kindly review it

@Planeshifter Planeshifter self-requested a review November 11, 2024 14:05
@Planeshifter Planeshifter changed the title feat: add math/base/special/lcmf feat: add math/base/special/lcmf Nov 11, 2024
@Planeshifter Planeshifter added the Needs Review A pull request which needs code review. label Nov 11, 2024
@gunjjoshi gunjjoshi added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Nov 11, 2024
@Harsh-Mathur-1503
Copy link
Author

@gunjjoshi I've done changes as requested by you
Kindly review it again for the same

Harsh-Mathur-1503 and others added 8 commits November 12, 2024 13:28
Co-authored-by: Gunj Joshi <[email protected]>
Signed-off-by: Harsh Mathur <[email protected]>
…nchmark.c

Co-authored-by: Gunj Joshi <[email protected]>
Signed-off-by: Harsh Mathur <[email protected]>
Replaced rand_int from rand_double as mentioned across other files

Signed-off-by: Harsh Mathur <[email protected]>
Signed-off-by: Harsh Mathur <[email protected]>
Signed-off-by: Harsh Mathur <[email protected]>
resolved import errors

Signed-off-by: Harsh Mathur <[email protected]>
resolved copy-paste error

Signed-off-by: Harsh Mathur <[email protected]>
@Harsh-Mathur-1503
Copy link
Author

@gunjjoshi Following issues were resolved.
Kindly review it again for the same.

Harsh-Mathur-1503 and others added 3 commits November 12, 2024 19:49
replaced space with tab indentation

Signed-off-by: Harsh Mathur <[email protected]>
Signed-off-by: Harsh Mathur <[email protected]>
@@ -0,0 +1,77 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Same goes for this too, we should follow the JS implementation of math/base/special/lcm here.

// MAIN //

/**
* Computes the least common multiple (LCM) of two floating-point numbers.
Copy link
Member

Choose a reason for hiding this comment

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

Here too, the description should match with what we have everywhere else.

Comment on lines +31 to +34
* @private
* @param {number} a - first floating-point number
* @param {number} b - second floating-point number
* @returns {number} least common multiple of `a` and `b`
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed as well.

@gunjjoshi
Copy link
Member

gunjjoshi commented Nov 12, 2024

@Harsh-Mathur-1503 You can once refer math/base/special/lcm, and try to match your PR with it, making precision changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Math Issue or pull request specific to math functionality. Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants