Skip to content

Fraction numerator denominator helpers#3605

Merged
gwhitney merged 22 commits intojosdejong:developfrom
AnslemHack:fraction-numerator-denominator-helpers
Mar 5, 2026
Merged

Fraction numerator denominator helpers#3605
gwhitney merged 22 commits intojosdejong:developfrom
AnslemHack:fraction-numerator-denominator-helpers

Conversation

@AnslemHack
Copy link
Copy Markdown
Contributor

@AnslemHack AnslemHack commented Dec 2, 2025

This PR fixes #3595 introduces two new arithmetic functions, num() and den(), which extract the numerator and denominator from Fraction objects respectively.

Copy link
Copy Markdown
Owner

@josdejong josdejong left a 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".

Comment thread src/function/arithmetic/num.js Outdated
Comment thread src/function/arithmetic/den.js Outdated
Comment thread src/function/arithmetic/num.js Outdated
Comment thread src/function/arithmetic/den.js Outdated
Comment thread types/index.d.ts Outdated
@AnslemHack
Copy link
Copy Markdown
Contributor Author

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

@josdejong
Copy link
Copy Markdown
Owner

Thanks!

@gwhitney
Copy link
Copy Markdown
Collaborator

gwhitney commented Dec 4, 2025

A few more items:

  • re() and im() are in src/function/complex. Shouldn't these be in a new subdirectory src/function/fraction? (Jos?)
  • Please add some tests showing that num and den do reasonable things on number and bigint input.
  • Presumably at the moment num and den will fail on BigNumber input, because there is no implicit conversion from BigNumber to Fraction. But it seems to me that for these two particular functions, what else could one mean but to convert the BigNumber to Fraction and then return the numerator or denominator? So, with @josdejong's blessing, please add an explicit signature for BigNumber for both functions that does the conversion and then returns the numerator or denominator. And then tests for this behavior, of course. Thanks!

@AnslemHack
Copy link
Copy Markdown
Contributor Author

A few more items:

  • re() and im() are in src/function/complex. Shouldn't these be in a new subdirectory src/function/fraction? (Jos?)
  • Please add some tests showing that num and den do reasonable things on number and bigint input.
  • Presumably at the moment num and den will fail on BigNumber input, because there is no implicit conversion from BigNumber to Fraction. But it seems to me that for these two particular functions, what else could one mean but to convert the BigNumber to Fraction and then return the numerator or denominator? So, with @josdejong's blessing, please add an explicit signature for BigNumber for both functions that does the conversion and then returns the numerator or denominator. And then tests for this behavior, of course. Thanks!

thanks for your feedback I have now addressed your concerns. please take another look.

Copy link
Copy Markdown
Collaborator

@gwhitney gwhitney left a 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!

Comment thread src/function/fraction/den.js Outdated
Comment thread types/index.d.ts Outdated
Comment thread types/index.d.ts Outdated
Comment thread types/index.d.ts
Comment thread test/typescript-tests/testTypes.ts
Comment thread test/unit-tests/function/fraction/den.test.js
@AnslemHack AnslemHack requested a review from gwhitney December 20, 2025 23:40
@AnslemHack
Copy link
Copy Markdown
Contributor Author

@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

@gwhitney
Copy link
Copy Markdown
Collaborator

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 develop branch, thanks. Should be ready to merge after that.

@AnslemHack
Copy link
Copy Markdown
Contributor Author

AnslemHack commented Jan 10, 2026

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 develop branch, thanks. Should be ready to merge after that.

@gwhitney @josdejong I have addressed most concerns now, mind taking another look in this pr?

Copy link
Copy Markdown
Collaborator

@gwhitney gwhitney left a comment

Choose a reason for hiding this comment

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

The new functions appear to comply with all of the provisions for adding a new function and to work as desired. Merging.

@gwhitney gwhitney merged commit f7c10b1 into josdejong:develop Mar 5, 2026
8 checks passed
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.

Provide access to numerator and denominator of fraction via num() and den() helpers

3 participants