Fraction numerator denominator helpers#3605
Fraction numerator denominator helpers#3605AnslemHack wants to merge 20 commits intojosdejong:developfrom
Conversation
josdejong
left a comment
josdejong
left a comment
There was a problem hiding this comment.
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
gwhitney
left a comment
There was a problem hiding this comment.
Thanks for great progress! Just a few things were overlooked or need to be brought into consistency with the rest of mathjs. Thanks!
src/function/fraction/den.js
Outdated
| 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.
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.
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.
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.
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 |
@gwhitney @josdejong I have addressed most concerns now, mind taking another look in this pr? |
3 participants
This PR fixes #3595 introduces two new arithmetic functions, num() and den(), which extract the numerator and denominator from Fraction objects respectively.