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

fix: Accept bigints in matrix indices and ranges, demoting to number #3361

Merged
merged 5 commits into from
Jan 30, 2025

Conversation

gwhitney
Copy link
Collaborator

Resolves #3360.

(Since it turns out to be such a small change, I just went ahead and implemented it. If in review there is any concern about the recommended change, to simply accept bigint indices and treat them as the corresponding numbers, it will be easy to modify or retract this.)

@josdejong
Copy link
Owner

Thanks Glen, looks good 👍.

I see a couple of occurrences in the comments of Range.js and range.js where we should update the param types, adding bigint (since the class and the function support number, BigNumber and bigint now. If you feel like it, it would be neat to update those. Otherwise, let's just merge this PR for now and address the comments later.

@gwhitney
Copy link
Collaborator Author

Oops, that comment has very much uncovered a bug. Clearly math.range(1n, 3n) should return math.matrix([1n, 2n]), but right now it is returning math.matrix([1, 2]). Marking this as draft until I fix it. Thanks for highlighting this!

@gwhitney gwhitney marked this pull request as draft January 29, 2025 19:33
@gwhitney gwhitney marked this pull request as ready for review January 29, 2025 20:09
@gwhitney
Copy link
Collaborator Author

OK, all should be well now. Note range() and Range() behave very differently with respect to bigints, echoing their pre-existing different handling of bignumbers: Range() objects are a bit second-class citizens in mathjs; there is not a lot of facility to create and manipulate them directly, and they pretty much only arise as internal objects used for Matrix/Array indexing. Since those indices can only be numbers, it makes sense that Range() objects can only contain numbers, and that any parameters supplied as bigints or bignumbers are demoted to numbers. On the other hand, range() is a first-class function, a utility for creating Matrix/Array objects with regularly spaced entries, and so the user might want results with entries of any mathjs numeric type, and hence it takes the cues of the type of the output from the type of the input.

In fact, from this point of view, range() not handling Fraction is an oversight. So marking this as draft again until I add Fraction support.

@gwhitney gwhitney marked this pull request as draft January 29, 2025 20:16
@gwhitney
Copy link
Collaborator Author

(In fact, there is decent argument that a range of two complex numbers should return a 2-dim Matrix of the rectangle with those complex numbers as opposite corners and steps as given in the real and imaginary dimensions, but then there are questions as to whether you should be able to separately supply real and imaginary steps, and if you supply a single complex step should you go back to just getting a vector of values, but then what's the stopping condition, so leaving that be for now. Not going to try to support Complex.)

@gwhitney
Copy link
Collaborator Author

OK, now range() supports Fraction, and as a bonus I unskipped the single-argument check. Should be ready for final review.

@gwhitney gwhitney marked this pull request as ready for review January 29, 2025 20:51
@josdejong
Copy link
Owner

Wow that involved more work than I had expected 😅 , thanks!

Ideally, I think whe should not have the same logic in both Range and range but unify this. Maybe Range is redundant.

@josdejong josdejong merged commit 1a85b87 into josdejong:develop Jan 30, 2025
8 checks passed
@josdejong
Copy link
Owner

Published now in v14.2.0.

@gwhitney
Copy link
Collaborator Author

Ι do not think Range is redundant, as it is lazy whereas range is eager. For long ranges, that can be important. If Range were ever made more prominent in the interface (as opposed to mostly hidden in the semantics for RangeNode) then I agree it should be enhanced to support other types. But right now it's only used for indexing arrays, I think, where only numbers make sense.

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.

Cannot index Array/Matrix with bigint
2 participants