-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fraction numerator denominator helpers #3605
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
base: develop
Are you sure you want to change the base?
Fraction numerator denominator helpers #3605
Conversation
josdejong
left a comment
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.
Thanks for working out these functions num and den @AnslemHack . Your PR looks well taken care of!
I made a few inline comments, can you have a look at those?
On a side note: the detailed description of your PR looks AI-generated and doesn't really add useful information to me 😅. I think it's enough to explain that "this PR addresses #3595, implementing two new functions, num and den".
my apologise for the verbose @josdejong , I must have over refined my output description, I have now made changes to that with a brief description of what it simply does, hope this is better |
|
Thanks! |
|
A few more items:
|
…nslemHack/mathjs into fraction-numerator-denominator-helpers
thanks for your feedback I have now addressed your concerns. please take another look. |
gwhitney
left a comment
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.
Thanks for great progress! Just a few things were overlooked or need to be brought into consistency with the rest of mathjs. Thanks!
| return typed(name, { | ||
| Fraction: (x) => x.d, | ||
| BigNumber: (x) => fraction(x).d, | ||
| 'Array | Matrix': typed.referToSelf((self) => (x) => deepMap(x, self)) |
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.
If you look throughout math.js, you will see the convention is to drop the parentheses in unary arrow-functions. E.g., this last one should be self => x => deepMap(s, self), and so on for several other examples throughout this PR. Please correct, thanks.
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.
There are still several examples of (onlyOneArgument) => functionBody rather than onlyOneArgument => functionBody in this PR; please convert to the latter format which is far more common throughout mathjs source code, thank you!
| const fractionMatrix = math.matrix([math.fraction(1, 2), math.fraction(3, 4)]) | ||
| expectTypeOf(math.num(fractionMatrix)).toMatchTypeOf<Matrix>() | ||
| expectTypeOf(math.den(fractionMatrix)).toMatchTypeOf<Matrix>() | ||
| assert.deepStrictEqual( |
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.
And you can put the more specific Matrix<bigint> type here, and add an example where you call num/den on an ordinary number or bigint. Thanks!
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.
Great, thanks! Just need to also put the <bigint> into .toMatchTypeOf<MathArray>() in a couple of places above as well.
|
@gwhitney @josdejong I believe to have addressed all concerns on this pr now.. would appreciate if you can have a look, still open for any further feedbacks or adjustment that might be required |
|
Yes, very close now. Just a couple of open items, and please in addition to taking care of those also update to be on top of the latest version of the |
This PR fixes #3595 introduces two new arithmetic functions, num() and den(), which extract the numerator and denominator from Fraction objects respectively.