Skip to content

fix: the internal number conversion inside Unit can throw an exception#3454

Merged
gwhitney merged 3 commits intodevelopfrom
fix/3450-unit-number-conversion
Apr 16, 2025
Merged

fix: the internal number conversion inside Unit can throw an exception#3454
gwhitney merged 3 commits intodevelopfrom
fix/3450-unit-number-conversion

Conversation

@josdejong
Copy link
Copy Markdown
Owner

@josdejong josdejong commented Apr 16, 2025

This is a workaround for #3450.

Note that for the .multiply method, it is possible to elimitate the temporarily introduced numbers 1, but for .divide that isn't possible, therefore I kept using the 1 values in both.

@gwhitney
Copy link
Copy Markdown
Collaborator

There are many reasons why it would be useful to have functions zero and one that return the additive and multiplicative identities, respectively, of the type of their argument. I've needed them in every new-engine prototype we've generated, for example to deal with generic Complex that can take an arbitrary type for its components. Current mathjs Unit represents some partway attempt to be generic over the type of the value; that's why the need is coming up here. (And frankly (A) had it been generic from the beginning, of course you would want to use the proper multiplicative identity in an operation, and (B) therefore Unit should be smoother if/when ported to one of the new-engine prototypes.)

Any interest in implementing zero and one in this PR and then using them in Unit for this purpose? That way this PR would become less of a hack/workaround, and be more constructive for the tentative development roadmap. Just a thought.

If you prefer I review this PR exactly as it is now, just let me know.

@josdejong
Copy link
Copy Markdown
Owner Author

Do you mean a function one which is used like one(4) returning 1 and one(bignumber(4)) returning bignumber(1)? Or do you have an other idea on how to pass the information on what numeric type to use?

@gwhitney
Copy link
Copy Markdown
Collaborator

Do you mean a function one which is used like one(4) returning 1 and one(bignumber(4)) returning bignumber(1)?

Precisely.

Or do you have an other idea on how to pass the information on what numeric type to use?

Well, in the very latest nanomath, one can also take a type object instead of an instance of the type, but there isn't really any analogue of that in mathjs right now.

@josdejong
Copy link
Copy Markdown
Owner Author

Ok I've now refactored the helper function convertToTypeOf into a more specific function one. Turning this into a full mathjs function one and creating a similar function zero will take some more work.

@gwhitney
Copy link
Copy Markdown
Collaborator

All right, good compromise. I will review now.

@gwhitney
Copy link
Copy Markdown
Collaborator

OK, updated HISTORY and made the new tests cover the other types besides bignumber. Will merge as soon as the CI completes.

@gwhitney gwhitney merged commit c3ee5ba into develop Apr 16, 2025
8 checks passed
@gwhitney gwhitney deleted the fix/3450-unit-number-conversion branch April 16, 2025 19:57
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.

2 participants